Re: [PATCH 07/22] plugins: Use emit_before_op for PLUGIN_GEN_AFTER_INSN
On 3/19/24 23:56, Richard Henderson wrote:
On 3/19/24 03:32, Pierrick Bouvier wrote:
static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
{
- TCGOp *op;
+ TCGOp *op, *next;
int insn_idx = -1;
pr_ops();
- QTAILQ_FOREACH(op, _ctx->ops, link) {
+ /*
+ * While injecting code, we cannot afford to reuse any ebb temps
+ * that might be live within the existing opcode stream.
+ * The simplest solution is to release them all and create new.
+ */
+ memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps));
+
Not an expert at this, but wouldn't that break an existing TB that already has
some ops on
those temps?
No, this only affects allocation of new temps -- if free_temps is empty, a new
temp will
be allocated from tcg_ctx->nb_temps++.
Zeroing free_temps here ensures that we *do not* reuse a temp that might
already be live
across any plugin insertion point. Between insertion points, we will free
plugin temps
and only reuse those.
Thanks, by looking at tcg_temp_new_internal fn, and with your
explaination, it makes more sense.
r~
Re: [PATCH 05/22] plugins: Move function pointer in qemu_plugin_dyn_cb
On 3/20/24 01:30, Richard Henderson wrote:
On 3/19/24 03:18, Pierrick Bouvier wrote:
On 3/16/24 05:57, Richard Henderson wrote:
The out-of-line function pointer is mutually exclusive
with inline expansion, so move it into the union.
Wrap the pointer in a structure named 'regular' to match
PLUGIN_CB_REGULAR.
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 4 +++-
accel/tcg/plugin-gen.c | 4 ++--
plugins/core.c | 8
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 12a96cea2a..143262dca8 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -84,13 +84,15 @@ enum plugin_dyn_cb_subtype {
* instance of a callback to be called upon the execution of a particular TB.
*/
struct qemu_plugin_dyn_cb {
- union qemu_plugin_cb_sig f;
void *userp;
enum plugin_dyn_cb_subtype type;
/* @rw applies to mem callbacks only (both regular and inline) */
enum qemu_plugin_mem_rw rw;
/* fields specific to each dyn_cb type go here */
union {
+ struct {
+ union qemu_plugin_cb_sig f;
+ } regular;
struct {
qemu_plugin_u64 entry;
enum qemu_plugin_op op;
While we are cleaning this, maybe this could be only a union (moving rw and
userp to
fields), and only type + union would be used.
Even if we duplicate userp in regular, and mem_cb, it would be much more
readable.
For instance, userp is never used by inline operations.
I was wondering about this. But I was also thinking about your follow-on patch
set to add
conditional calls: do we want a multiplicity of union members, or will we want
separate
bits and pieces that can be strung together?
E.g.
TCGCond cond; /* ALWAYS, or compare entry vs imm. */
/* PLUGIN_CB_REGULAR_COND or PLUGIN_CB_INLINE_* */
qemu_plugin_u64 entry;
uint64_t imm;
/* PLUGIN_CB_REGULAR_* */
union qemu_plugin_cb_sig f;
void *userp;
In an ideal world:
struct regular_cb {
// full data for regular cb
};
struct conditional_cb {
// full data for cond cb
};
struct inline_op {
// full data for inline op
};
...
and
struct qemu_plugin_dyn_cb {
enum plugin_dyn_cb_subtype type;
union {
struct regular_cb regular;
struct conditional_cb conditional;
struct inline_op inline;
};
};
It's the same as describing the structs directly inside union, but at
least you can duplicate fields without thinking too much. In terms of
memory layout it does not change anything, the upper bound is still the
largest struct, whether you share fields or not.
In short, an algebraic data type. Hope it's not a bad word around here :)
r~
Re: [PATCH 06/22] plugins: Create TCGHelperInfo for all out-of-line callbacks
On 3/19/24 23:51, Richard Henderson wrote:
On 3/19/24 03:12, Pierrick Bouvier wrote:
On 3/16/24 05:57, Richard Henderson wrote:
TCGHelperInfo includes the ABI for every function call.
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 1 +
plugins/core.c | 51 ++-
2 files changed, 46 insertions(+), 6 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 143262dca8..793c44f1f2 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -92,6 +92,7 @@ struct qemu_plugin_dyn_cb {
union {
struct {
union qemu_plugin_cb_sig f;
+ TCGHelperInfo *info;
} regular;
struct {
qemu_plugin_u64 entry;
diff --git a/plugins/core.c b/plugins/core.c
index 837c373690..b0a2e80874 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -338,12 +338,26 @@ void plugin_register_dyn_cb__udata(GArray **arr,
enum qemu_plugin_cb_flags flags,
void *udata)
{
- struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr);
+ static TCGHelperInfo info[3] = {
+ [QEMU_PLUGIN_CB_NO_REGS].flags = TCG_CALL_NO_RWG | TCG_CALL_PLUGIN,
+ [QEMU_PLUGIN_CB_R_REGS].flags = TCG_CALL_NO_WG | TCG_CALL_PLUGIN,
+ [QEMU_PLUGIN_CB_RW_REGS].flags = TCG_CALL_PLUGIN,
+ /*
+ * Match qemu_plugin_vcpu_udata_cb_t:
+ * void (*)(uint32_t, void *)
+
Any chance we could have a static assert ensuring this?
I know it's possible in C11, but I don't know if glib offers something for this.
I don't see how. While you could ask questions about the pointer type
qemu_plugin_vcpu_udata_cb_t, you can't ask questions about the function
arguments.
I was thinking about something similar to:
static_assert(typeof(qemu_plugin_vcpu_udata_cb_t) == void (*)(uint32_t,
void *));
But I don't think it's possible to express this in C standard before 11.
r~
[PATCH] target/i386: Check NULL monitor pointer when injecting MCE
monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
may have a parameter with NULL monitor pointer. Check the monitor pointer
before calling monitor_puts().
Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
Reviwed-by: Xiaoyao Li
Signed-off-by: Tao Su
---
target/i386/helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/i386/helper.c b/target/i386/helper.c
index 2070dd0dda..a9ff830a17 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -430,7 +430,8 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data
data)
if (need_reset) {
emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
recursive);
-monitor_puts(params->mon, msg);
+if (params->mon)
+monitor_puts(params->mon, msg);
qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
return;
--
2.34.1
Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
On 20/03/24 3:27 am, Peter Xu wrote:
On Tue, Mar 19, 2024 at 08:48:39PM +, Het Gala wrote:
'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.
Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.
Hmm, this patch isn't as clear to me. Even if this can return NULL now,
it'll soon crash at some later point, no?
It won't crash IMO, the next function SocketAddress_to_str for a non-socket
type would return an proper error ?
IMHO such patch is more suitable to be included in the same patch where
such new tests will be introduced, then we're addressing some real test
code changes that will work, rather than worrying on what will happen in
the future (and as I mentioned, i don't think it fully resolved that either..)
Thanks,
Maybe, Fabiano can pick this patch, and add along file migration qtests
patch ?
Suggested-by: Fabiano Rosas
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..fb7156f09a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState
*who)
QObject *object;
rsp = migrate_query(who);
+
+if (!qdict_haskey(rsp, "socket-address")) {
+return NULL;
+}
object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
--
2.22.3
Regards,
Het Gala
Re: [PATCH-for-9.1 19/21] target/ppc: Factor ppc_add_alias_definitions() out
On Fri Mar 15, 2024 at 11:09 PM AEST, Philippe Mathieu-Daudé wrote:
> Factor ppc_add_alias_definitions() out of qmp_query_cpu_definitions()
> to clearly see the generic pattern used in all targets.
Looks equivalent.
Reviewed-by: Nicholas Piggin
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/ppc/cpu-models.h | 4
> target/ppc/ppc-qmp-cmds.c | 26 +++---
> 2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/target/ppc/cpu-models.h b/target/ppc/cpu-models.h
> index 0229ef3a9a..89a5e232b7 100644
> --- a/target/ppc/cpu-models.h
> +++ b/target/ppc/cpu-models.h
> @@ -21,6 +21,8 @@
> #ifndef TARGET_PPC_CPU_MODELS_H
> #define TARGET_PPC_CPU_MODELS_H
>
> +#include "qapi/qapi-types-machine.h"
> +
> /**
> * PowerPCCPUAlias:
> * @alias: The alias name.
> @@ -480,4 +482,6 @@ enum {
> POWERPC_SVR_8641D = 0x80900121,
> };
>
> +void ppc_add_alias_definitions(CpuDefinitionInfoList **cpu_list);
> +
> #endif
> diff --git a/target/ppc/ppc-qmp-cmds.c b/target/ppc/ppc-qmp-cmds.c
> index a25d86a8d1..528cc3e4af 100644
> --- a/target/ppc/ppc-qmp-cmds.c
> +++ b/target/ppc/ppc-qmp-cmds.c
> @@ -189,17 +189,9 @@ static void ppc_cpu_defs_entry(gpointer data, gpointer
> user_data)
> QAPI_LIST_PREPEND(*first, info);
> }
>
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +void ppc_add_alias_definitions(CpuDefinitionInfoList **cpu_list)
> {
> -CpuDefinitionInfoList *cpu_list = NULL;
> -GSList *list;
> -int i;
> -
> -list = object_class_get_list(TYPE_POWERPC_CPU, false);
> -g_slist_foreach(list, ppc_cpu_defs_entry, _list);
> -g_slist_free(list);
> -
> -for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
> +for (unsigned i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
> PowerPCCPUAlias *alias = _cpu_aliases[i];
> ObjectClass *oc;
> CpuDefinitionInfo *info;
> @@ -213,8 +205,20 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error
> **errp)
> info->name = g_strdup(alias->alias);
> info->q_typename = g_strdup(object_class_get_name(oc));
>
> -QAPI_LIST_PREPEND(cpu_list, info);
> +QAPI_LIST_PREPEND(*cpu_list, info);
> }
> +}
> +
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +{
> +CpuDefinitionInfoList *cpu_list = NULL;
> +GSList *list;
> +
> +list = object_class_get_list(TYPE_POWERPC_CPU, false);
> +g_slist_foreach(list, ppc_cpu_defs_entry, _list);
> +g_slist_free(list);
> +
> +ppc_add_alias_definitions(_list);
>
> return cpu_list;
> }
Re: [PATCH v2 1/2] target/ppc: Merge various fpu helpers
On Fri Mar 15, 2024 at 4:44 PM AEST, Chinmay Rath wrote:
> This patch merges the definitions of the following set of fpu helper methods,
> which are similar, using macros :
>
> 1. f{add, sub, mul, div}(s)
> 2. fre(s)
> 3. frsqrte(s)
>
Reviewed-by: Nicholas Piggin
> Signed-off-by: Chinmay Rath
> ---
> target/ppc/fpu_helper.c | 221 +++-
> 1 file changed, 62 insertions(+), 159 deletions(-)
>
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 4b3dcad5d1..8d0cbe27e7 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -490,54 +490,12 @@ static void float_invalid_op_addsub(CPUPPCState *env,
> int flags,
> }
> }
>
> -/* fadd - fadd. */
> -float64 helper_fadd(CPUPPCState *env, float64 arg1, float64 arg2)
> +static inline void addsub_flags_handler(CPUPPCState *env, int flags,
> +uintptr_t ra)
> {
> -float64 ret = float64_add(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> -if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_addsub(env, flags, 1, GETPC());
> -}
> -
> -return ret;
> -}
> -
> -/* fadds - fadds. */
> -float64 helper_fadds(CPUPPCState *env, float64 arg1, float64 arg2)
> -{
> -float64 ret = float64r32_add(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> -if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_addsub(env, flags, 1, GETPC());
> -}
> -return ret;
> -}
> -
> -/* fsub - fsub. */
> -float64 helper_fsub(CPUPPCState *env, float64 arg1, float64 arg2)
> -{
> -float64 ret = float64_sub(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_addsub(env, flags, 1, GETPC());
> +float_invalid_op_addsub(env, flags, 1, ra);
> }
> -
> -return ret;
> -}
> -
> -/* fsubs - fsubs. */
> -float64 helper_fsubs(CPUPPCState *env, float64 arg1, float64 arg2)
> -{
> -float64 ret = float64r32_sub(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> -if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_addsub(env, flags, 1, GETPC());
> -}
> -return ret;
> }
>
> static void float_invalid_op_mul(CPUPPCState *env, int flags,
> @@ -550,29 +508,11 @@ static void float_invalid_op_mul(CPUPPCState *env, int
> flags,
> }
> }
>
> -/* fmul - fmul. */
> -float64 helper_fmul(CPUPPCState *env, float64 arg1, float64 arg2)
> -{
> -float64 ret = float64_mul(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> -if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_mul(env, flags, 1, GETPC());
> -}
> -
> -return ret;
> -}
> -
> -/* fmuls - fmuls. */
> -float64 helper_fmuls(CPUPPCState *env, float64 arg1, float64 arg2)
> +static inline void mul_flags_handler(CPUPPCState *env, int flags, uintptr_t
> ra)
> {
> -float64 ret = float64r32_mul(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_mul(env, flags, 1, GETPC());
> +float_invalid_op_mul(env, flags, 1, ra);
> }
> -return ret;
> }
>
> static void float_invalid_op_div(CPUPPCState *env, int flags,
> @@ -587,36 +527,14 @@ static void float_invalid_op_div(CPUPPCState *env, int
> flags,
> }
> }
>
> -/* fdiv - fdiv. */
> -float64 helper_fdiv(CPUPPCState *env, float64 arg1, float64 arg2)
> -{
> -float64 ret = float64_div(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> -if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_div(env, flags, 1, GETPC());
> -}
> -if (unlikely(flags & float_flag_divbyzero)) {
> -float_zero_divide_excp(env, GETPC());
> -}
> -
> -return ret;
> -}
> -
> -/* fdivs - fdivs. */
> -float64 helper_fdivs(CPUPPCState *env, float64 arg1, float64 arg2)
> +static inline void div_flags_handler(CPUPPCState *env, int flags, uintptr_t
> ra)
> {
> -float64 ret = float64r32_div(arg1, arg2, >fp_status);
> -int flags = get_float_exception_flags(>fp_status);
> -
> if (unlikely(flags & float_flag_invalid)) {
> -float_invalid_op_div(env, flags, 1, GETPC());
> +float_invalid_op_div(env, flags, 1, ra);
> }
> if (unlikely(flags & float_flag_divbyzero)) {
> -float_zero_divide_excp(env, GETPC());
> +float_zero_divide_excp(env, ra);
> }
> -
> -return ret;
> }
>
> static uint64_t float_invalid_cvt(CPUPPCState *env, int flags,
> @@ -812,81 +730,66 @@ float64 helper_##name(CPUPPCState *env, float64 arg)
> \
> FPU_FSQRT(FSQRT, float64_sqrt)
> FPU_FSQRT(FSQRTS, float64r32_sqrt)
>
> -/* fre - fre. */
>
Re: [PATCH] target/ppc/mmu-radix64: Use correct string format in walk_tree()
Thanks, I can put this in the ppc tree.
Thanks,
Nick
On Tue Mar 19, 2024 at 4:30 PM AEST, Philippe Mathieu-Daudé wrote:
> +Anton
>
> On 19/3/24 06:10, Philippe Mathieu-Daudé wrote:
> > 'mask', 'nlb' and 'base_addr' are all uin64_t types.
> > Use the corresponding PRIx64 format.
> >
> > Fixes: d2066bc50d ("target/ppc: Check page dir/table base alignment")
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > target/ppc/mmu-radix64.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> > index 5823e039e6..690dff7a49 100644
> > --- a/target/ppc/mmu-radix64.c
> > +++ b/target/ppc/mmu-radix64.c
> > @@ -300,8 +300,8 @@ static int ppc_radix64_next_level(AddressSpace *as,
> > vaddr eaddr,
> >
> > if (nlb & mask) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > -"%s: misaligned page dir/table base: 0x"TARGET_FMT_lx
> > -" page dir size: 0x"TARGET_FMT_lx"\n",
> > +"%s: misaligned page dir/table base: 0x%" PRIx64
> > +" page dir size: 0x%" PRIx64 "\n",
> > __func__, nlb, mask + 1);
> > nlb &= ~mask;
> > }
> > @@ -324,8 +324,8 @@ static int ppc_radix64_walk_tree(AddressSpace *as,
> > vaddr eaddr,
> >
> > if (base_addr & mask) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > -"%s: misaligned page dir base: 0x"TARGET_FMT_lx
> > -" page dir size: 0x"TARGET_FMT_lx"\n",
> > +"%s: misaligned page dir base: 0x%" PRIx64
> > +" page dir size: 0x%" PRIx64 "\n",
> > __func__, base_addr, mask + 1);
> > base_addr &= ~mask;
> > }
Re: [PATCH-for-9.1 18/27] target/ppc: Convert to TCGCPUOps::get_cpu_state()
On Wed Mar 20, 2024 at 1:42 AM AEST, Philippe Mathieu-Daudé wrote:
> Convert cpu_get_tb_cpu_state() to TCGCPUOps::get_cpu_state(),
> unifying with the method declared in target/ppc/helper_regs.c.
Looks okay AFAIKS.
Reviewed-by: Nicholas Piggin
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/ppc/cpu.h | 16 +++-
> target/ppc/cpu_init.c| 1 +
> target/ppc/helper_regs.c | 13 +++--
> 3 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index ced4e53024..6aa18db335 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2716,19 +2716,9 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer);
> */
> #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))
>
> -#define TARGET_HAS_CPU_GET_TB_CPU_STATE
> -
> -#ifdef CONFIG_DEBUG_TCG
> -void cpu_get_tb_cpu_state(CPUPPCState *env, vaddr *pc,
> - uint64_t *cs_base, uint32_t *flags);
> -#else
> -static inline void cpu_get_tb_cpu_state(CPUPPCState *env, vaddr *pc,
> -uint64_t *cs_base, uint32_t *flags)
> -{
> -*pc = env->nip;
> -*cs_base = 0;
> -*flags = env->hflags;
> -}
> +#ifdef CONFIG_TCG
> +void ppc_get_cpu_state(CPUPPCState *env, vaddr *pc,
> + uint64_t *cs_base, uint32_t *flags);
> #endif
>
> G_NORETURN void raise_exception(CPUPPCState *env, uint32_t exception);
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 464e91faa2..673559b444 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7362,6 +7362,7 @@ static const struct SysemuCPUOps ppc_sysemu_ops = {
> static const TCGCPUOps ppc_tcg_ops = {
> .initialize = ppc_translate_init,
> .restore_state_to_opc = ppc_restore_state_to_opc,
> +.get_cpu_state = ppc_get_cpu_state,
>
> #ifdef CONFIG_USER_ONLY
> .record_sigsegv = ppc_cpu_record_sigsegv,
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 25258986e3..e62591067c 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -217,25 +217,26 @@ void hreg_update_pmu_hflags(CPUPPCState *env)
> env->hflags |= hreg_compute_pmu_hflags_value(env);
> }
>
> -#ifdef CONFIG_DEBUG_TCG
> -void cpu_get_tb_cpu_state(CPUPPCState *env, vaddr *pc,
> - uint64_t *cs_base, uint32_t *flags)
> +#ifdef CONFIG_TCG
> +void ppc_get_cpu_state(CPUPPCState *env, vaddr *pc,
> + uint64_t *cs_base, uint32_t *flags)
> {
> uint32_t hflags_current = env->hflags;
> -uint32_t hflags_rebuilt;
>
> *pc = env->nip;
> *cs_base = 0;
> *flags = hflags_current;
>
> -hflags_rebuilt = hreg_compute_hflags_value(env);
> +#ifdef CONFIG_DEBUG_TCG
> +uint32_t hflags_rebuilt = hreg_compute_hflags_value(env);
> if (unlikely(hflags_current != hflags_rebuilt)) {
> cpu_abort(env_cpu(env),
>"TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
>hflags_current, hflags_rebuilt);
> }
> -}
> #endif
> +}
> +#endif /* CONFIG_TCG */
>
> void cpu_interrupt_exittb(CPUState *cs)
> {
Re: [PATCH for 9.0 v15 00/10] target/riscv: vector fixes
On Fri, Mar 15, 2024 at 3:59 AM Daniel Henrique Barboza wrote: > > Hi, > > The series was renamed to reflect that at this point we're fixing more > things than just vstart management. > > In this new version a couple fixes were added: > > - patch 3 (new) fixes the memcpy endianess in 'vmvr_v', as suggested by > Richard; > > - patch 5 (new) fixes ldst_whole insns to now clear vstart in all cases. > The fix was proposed by Max. > > Another notable change was made in patch 6 (patch 4 from v14). We're not > doing early exits in helpers that are gated by vstart_eq_zero. This was > found to cause side-effects with insns that wants to send faults if vl = > 0, and for the rest it becomes a moot check since vstart is granted to > be zero beforehand. > > Series based on master. > > Patches missing acks: 3, 4, 5 > > Changes from v14: > - patch 3 (new): > - make 'vmvr_v' big endian compliant > - patch 5 (new): > - make ldst_whole insns clear vstart in all code paths > - patch 6 (patch 4 from v14): > - do not add early exits on helpers that are gated with vstart_eq_zero > - v14 link: > https://lore.kernel.org/qemu-riscv/20240313220141.427730-1-dbarb...@ventanamicro.com/ > > Daniel Henrique Barboza (9): > target/riscv/vector_helper.c: set vstart = 0 in GEN_VEXT_VSLIDEUP_VX() > trans_rvv.c.inc: set vstart = 0 in int scalar move insns > target/riscv/vector_helper.c: fix 'vmvr_v' memcpy endianess > target/riscv: always clear vstart in whole vec move insns > target/riscv: always clear vstart for ldst_whole insns > target/riscv/vector_helpers: do early exit when vstart >= vl > target/riscv: remove 'over' brconds from vector trans > trans_rvv.c.inc: remove redundant mark_vs_dirty() calls > target/riscv/vector_helper.c: optimize loops in ldst helpers > > Ivan Klokov (1): > target/riscv: enable 'vstart_eq_zero' in the end of insns Thanks! Applied to riscv-to-apply.next Alistair > > target/riscv/insn_trans/trans_rvbf16.c.inc | 18 +- > target/riscv/insn_trans/trans_rvv.c.inc| 244 ++--- > target/riscv/insn_trans/trans_rvvk.c.inc | 30 +-- > target/riscv/translate.c | 6 + > target/riscv/vcrypto_helper.c | 32 +++ > target/riscv/vector_helper.c | 93 +++- > target/riscv/vector_internals.c| 4 + > target/riscv/vector_internals.h| 9 + > 8 files changed, 220 insertions(+), 216 deletions(-) > > -- > 2.44.0 > >
Re: [PATCH v5 5/5] target/riscv: Implement privilege mode filtering for cycle/instret
On Thu, Mar 7, 2024 at 7:26 PM Atish Patra wrote:
>
>
> On 3/4/24 22:47, LIU Zhiwei wrote:
> >
> > On 2024/2/29 2:51, Atish Patra wrote:
> >> Privilege mode filtering can also be emulated for cycle/instret by
> >> tracking host_ticks/icount during each privilege mode switch. This
> >> patch implements that for both cycle/instret and mhpmcounters. The
> >> first one requires Smcntrpmf while the other one requires Sscofpmf
> >> to be enabled.
> >>
> >> The cycle/instret are still computed using host ticks when icount
> >> is not enabled. Otherwise, they are computed using raw icount which
> >> is more accurate in icount mode.
> >>
> >> Reviewed-by: Daniel Henrique Barboza
> >> Signed-off-by: Atish Patra
> >> ---
> >> target/riscv/cpu.h| 11 +
> >> target/riscv/cpu_bits.h | 5 ++
> >> target/riscv/cpu_helper.c | 17 ++-
> >> target/riscv/csr.c| 96 ++-
> >> target/riscv/pmu.c| 64 ++
> >> target/riscv/pmu.h| 2 +
> >> 6 files changed, 171 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 174e8ba8e847..9e21d7f7d635 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -157,6 +157,15 @@ typedef struct PMUCTRState {
> >> target_ulong irq_overflow_left;
> >> } PMUCTRState;
> >> +typedef struct PMUFixedCtrState {
> >> +/* Track cycle and icount for each privilege mode */
> >> +uint64_t counter[4];
> >> +uint64_t counter_prev[4];
> >> +/* Track cycle and icount for each privilege mode when V = 1*/
> >> +uint64_t counter_virt[2];
> >> +uint64_t counter_virt_prev[2];
> >> +} PMUFixedCtrState;
> >> +
> >> struct CPUArchState {
> >> target_ulong gpr[32];
> >> target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> >> @@ -353,6 +362,8 @@ struct CPUArchState {
> >> /* PMU event selector configured values for RV32 */
> >> target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> >> +PMUFixedCtrState pmu_fixed_ctrs[2];
> >> +
> >> target_ulong sscratch;
> >> target_ulong mscratch;
> >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> >> index e866c60a400c..5fe349e313dc 100644
> >> --- a/target/riscv/cpu_bits.h
> >> +++ b/target/riscv/cpu_bits.h
> >> @@ -920,6 +920,11 @@ typedef enum RISCVException {
> >> #define MHPMEVENT_BIT_VUINHBIT_ULL(58)
> >> #define MHPMEVENTH_BIT_VUINH BIT(26)
> >> +#define MHPMEVENT_FILTER_MASK (MHPMEVENT_BIT_MINH | \
> >> +MHPMEVENT_BIT_SINH | \
> >> +MHPMEVENT_BIT_UINH | \
> >> +MHPMEVENT_BIT_VSINH | \
> >> + MHPMEVENT_BIT_VUINH)
> >> #define MHPMEVENT_SSCOF_MASK _ULL(0x)
> >> #define MHPMEVENT_IDX_MASK 0xF
> >> #define MHPMEVENT_SSCOF_RESVD 16
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index d462d95ee165..33965d843d46 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -718,8 +718,21 @@ void riscv_cpu_set_mode(CPURISCVState *env,
> >> target_ulong newpriv)
> >> {
> >> g_assert(newpriv <= PRV_M && newpriv != PRV_RESERVED);
> >> -if (icount_enabled() && newpriv != env->priv) {
> >> -riscv_itrigger_update_priv(env);
> >> +/*
> >> + * Invoke cycle/instret update between priv mode changes or
> >> + * VS->HS mode transition is SPV bit must be set
> >> + * HS->VS mode transition where virt_enabled must be set
> >> + * In both cases, priv will S mode only.
> >> + */
> >> +if (newpriv != env->priv ||
> >> + (env->priv == PRV_S && newpriv == PRV_S &&
> >> +(env->virt_enabled || get_field(env->hstatus, HSTATUS_SPV {
> >> +if (icount_enabled()) {
> >> +riscv_itrigger_update_priv(env);
> >> +riscv_pmu_icount_update_priv(env, newpriv);
> >> +} else {
> >> +riscv_pmu_cycle_update_priv(env, newpriv);
> >> +}
> >> }
> >> /* tlb_flush is unnecessary as mode is contained in mmu_idx */
> >> env->priv = newpriv;
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index ff9bac537593..482e212c5f74 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -788,32 +788,16 @@ static RISCVException write_vcsr(CPURISCVState
> >> *env, int csrno,
> >> return RISCV_EXCP_NONE;
> >> }
> >> +#if defined(CONFIG_USER_ONLY)
> >> /* User Timers and Counters */
> >> static target_ulong get_ticks(bool shift)
> >> {
> >> -int64_t val;
> >> -target_ulong result;
> >> -
> >> -#if !defined(CONFIG_USER_ONLY)
> >> -if (icount_enabled()) {
> >> -val = icount_get();
> >> -} else {
> >> -val = cpu_get_host_ticks();
> >> -
Re: [PATCH-for-9.1 17/27] target/ppc: Indent ppc_tcg_ops[] with 4 spaces
Acked-by: Nicholas Piggin
On Wed Mar 20, 2024 at 1:42 AM AEST, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/ppc/cpu_init.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 7e65f08147..464e91faa2 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7360,22 +7360,22 @@ static const struct SysemuCPUOps ppc_sysemu_ops = {
> #include "hw/core/tcg-cpu-ops.h"
>
> static const TCGCPUOps ppc_tcg_ops = {
> - .initialize = ppc_translate_init,
> - .restore_state_to_opc = ppc_restore_state_to_opc,
> +.initialize = ppc_translate_init,
> +.restore_state_to_opc = ppc_restore_state_to_opc,
>
> #ifdef CONFIG_USER_ONLY
> - .record_sigsegv = ppc_cpu_record_sigsegv,
> +.record_sigsegv = ppc_cpu_record_sigsegv,
> #else
> - .tlb_fill = ppc_cpu_tlb_fill,
> - .cpu_exec_interrupt = ppc_cpu_exec_interrupt,
> - .do_interrupt = ppc_cpu_do_interrupt,
> - .cpu_exec_enter = ppc_cpu_exec_enter,
> - .cpu_exec_exit = ppc_cpu_exec_exit,
> - .do_unaligned_access = ppc_cpu_do_unaligned_access,
> - .do_transaction_failed = ppc_cpu_do_transaction_failed,
> - .debug_excp_handler = ppc_cpu_debug_excp_handler,
> - .debug_check_breakpoint = ppc_cpu_debug_check_breakpoint,
> - .debug_check_watchpoint = ppc_cpu_debug_check_watchpoint,
> +.tlb_fill = ppc_cpu_tlb_fill,
> +.cpu_exec_interrupt = ppc_cpu_exec_interrupt,
> +.do_interrupt = ppc_cpu_do_interrupt,
> +.cpu_exec_enter = ppc_cpu_exec_enter,
> +.cpu_exec_exit = ppc_cpu_exec_exit,
> +.do_unaligned_access = ppc_cpu_do_unaligned_access,
> +.do_transaction_failed = ppc_cpu_do_transaction_failed,
> +.debug_excp_handler = ppc_cpu_debug_excp_handler,
> +.debug_check_breakpoint = ppc_cpu_debug_check_breakpoint,
> +.debug_check_watchpoint = ppc_cpu_debug_check_watchpoint,
> #endif /* !CONFIG_USER_ONLY */
> };
> #endif /* CONFIG_TCG */
Re: [PATCH v5 08/24] replay: Fix migration use of clock
On Wed Mar 20, 2024 at 6:40 AM AEST, Alex Bennée wrote: > Nicholas Piggin writes: > > > Migration reads host clocks when not holding the replay_mutex, which > > asserts when recording a trace. It seems that these migration times > > should be host times like other statistics in MigrationState. > > s/host/CLOCK_HOST/ and s/host/CLOCK_REALTIME/ but its a confusing > sentence. Yes you're right. > If the MigrationState is guest visible it should be > QEMU_CLOCK_VIRTUAL surely? I didn't think it was visible. It was added with ed4fbd10823 and just exported to QMP. It was the first and only user of host clock in migration stats, the rest use rt clock. OTOH it does seem to have deliberately chosen host... can you see any reason for it? Thanks, Nick
Re: [PATCH for 9.0 v15 06/10] target/riscv/vector_helpers: do early exit when vstart >= vl
On Fri, Mar 15, 2024 at 3:59 AM Daniel Henrique Barboza
wrote:
>
> We're going to make changes that will required each helper to be
> responsible for the 'vstart' management, i.e. we will relieve the
> 'vstart < vl' assumption that helpers have today.
>
> Helpers are usually able to deal with vstart >= vl, i.e. doing nothing
> aside from setting vstart = 0 at the end, but the tail update functions
> will update the tail regardless of vstart being valid or not. Unifying
> the tail update process in a single function that would handle the
> vstart >= vl case isn't trivial (see [1] for more info).
>
> This patch takes a blunt approach: do an early exit in every single
> vector helper if vstart >= vl, unless the helper is guarded with
> vstart_eq_zero in the translation. For those cases the helper is ready
> to deal with cases where vl might be zero, i.e. throwing exceptions
> based on it like vcpop_m() and first_m().
>
> Helpers that weren't changed:
>
> - vcpop_m(), vfirst_m(), vmsetm(), GEN_VEXT_VIOTA_M(): these are guarded
> directly with vstart_eq_zero;
>
> - GEN_VEXT_VCOMPRESS_VM(): guarded with vcompress_vm_check() that checks
> vstart_eq_zero;
>
> - GEN_VEXT_RED(): guarded with either reduction_check() or
> reduction_widen_check(), both check vstart_eq_zero;
>
> - GEN_VEXT_FRED(): guarded with either freduction_check() or
> freduction_widen_check(), both check vstart_eq_zero.
>
> Another exception is vext_ldst_whole(), who operates on effective vector
> length regardless of the current settings in vtype and vl.
>
> [1]
> https://lore.kernel.org/qemu-riscv/1590234b-0291-432a-a0fa-c5a687609...@linux.alibaba.com/
>
> Signed-off-by: Daniel Henrique Barboza
> Reviewed-by: Richard Henderson
> Signed-off-by: Daniel Henrique Barboza
Acked-by: Alistair Francis
Alistair
> ---
> target/riscv/vcrypto_helper.c | 32
> target/riscv/vector_helper.c| 66 +
> target/riscv/vector_internals.c | 4 ++
> target/riscv/vector_internals.h | 9 +
> 4 files changed, 111 insertions(+)
>
> diff --git a/target/riscv/vcrypto_helper.c b/target/riscv/vcrypto_helper.c
> index e2d719b13b..f7423df226 100644
> --- a/target/riscv/vcrypto_helper.c
> +++ b/target/riscv/vcrypto_helper.c
> @@ -222,6 +222,8 @@ static inline void xor_round_key(AESState *round_state,
> AESState *round_key)
> uint32_t total_elems = vext_get_total_elems(env, desc, 4);\
> uint32_t vta = vext_vta(desc);\
>\
> +VSTART_CHECK_EARLY_EXIT(env); \
> + \
> for (uint32_t i = env->vstart / 4; i < env->vl / 4; i++) {\
> AESState round_key; \
> round_key.d[0] = *((uint64_t *)vs2 + H8(i * 2 + 0)); \
> @@ -246,6 +248,8 @@ static inline void xor_round_key(AESState *round_state,
> AESState *round_key)
> uint32_t total_elems = vext_get_total_elems(env, desc, 4);\
> uint32_t vta = vext_vta(desc);\
>\
> +VSTART_CHECK_EARLY_EXIT(env); \
> + \
> for (uint32_t i = env->vstart / 4; i < env->vl / 4; i++) {\
> AESState round_key; \
> round_key.d[0] = *((uint64_t *)vs2 + H8(0)); \
> @@ -305,6 +309,8 @@ void HELPER(vaeskf1_vi)(void *vd_vptr, void *vs2_vptr,
> uint32_t uimm,
> uint32_t total_elems = vext_get_total_elems(env, desc, 4);
> uint32_t vta = vext_vta(desc);
>
> +VSTART_CHECK_EARLY_EXIT(env);
> +
> uimm &= 0b;
> if (uimm > 10 || uimm == 0) {
> uimm ^= 0b1000;
> @@ -351,6 +357,8 @@ void HELPER(vaeskf2_vi)(void *vd_vptr, void *vs2_vptr,
> uint32_t uimm,
> uint32_t total_elems = vext_get_total_elems(env, desc, 4);
> uint32_t vta = vext_vta(desc);
>
> +VSTART_CHECK_EARLY_EXIT(env);
> +
> uimm &= 0b;
> if (uimm > 14 || uimm < 2) {
> uimm ^= 0b1000;
> @@ -457,6 +465,8 @@ void HELPER(vsha2ms_vv)(void *vd, void *vs1, void *vs2,
> CPURISCVState *env,
> uint32_t total_elems;
> uint32_t vta = vext_vta(desc);
>
> +VSTART_CHECK_EARLY_EXIT(env);
> +
> for (uint32_t i = env->vstart / 4; i < env->vl / 4; i++) {
> if (sew == MO_32) {
> vsha2ms_e32(((uint32_t *)vd) + i * 4, ((uint32_t *)vs1) + i * 4,
> @@ -572,6 +582,8 @@ void HELPER(vsha2ch32_vv)(void *vd, void *vs1, void *vs2,
> CPURISCVState *env,
> uint32_t total_elems;
> uint32_t vta = vext_vta(desc);
>
> +VSTART_CHECK_EARLY_EXIT(env);
> +
>
Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
On Thu, Mar 7, 2024 at 7:22 PM Daniel Henrique Barboza
wrote:
>
>
>
> On 3/7/24 00:45, Sunil V L wrote:
> > On Thu, Mar 07, 2024 at 11:33:25AM +1000, Alistair Francis wrote:
> >> On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> This patch break check-qtest, most specifically 'bios-table'test', for
> >>> aarch64.
> >>> I found this while running riscv-to-apply.next in the Gitlab pipeline.
> >>>
> >>>
> >>> Here's the output:
> >>>
> >>> $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1
> >>> ./tests/qtest/bios-tables-test
> >>> TAP version 13
> >>> # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d
> >>> # starting QEMU: exec ./qemu-system-aarch64 -qtest
> >>> unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev
> >>> socket,path=/tmp/qtest-591353.qmp,id=char0 -mon
> >>> chardev=char0,mode=control -display none -audio none -machine none -accel
> >>> qtest
> >>> 1..8
> >>> # Start of aarch64 tests
> >>> # Start of acpi tests
> >>> # starting QEMU: exec ./qemu-system-aarch64 -qtest
> >>> unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev
> >>> socket,path=/tmp/qtest-591353.qmp,id=char0 -mon
> >>> chardev=char0,mode=control -display none -audio none -machine virt
> >>> -accel tcg -nodefaults -nographic -drive
> >>> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive
> >>> if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom
> >>> tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu
> >>> cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest
> >>> acpi-test: Warning! SPCR binary file mismatch. Actual
> >>> [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR].
> >>> See source file tests/qtest/bios-tables-test.c for instructions on how to
> >>> update expected files.
> >>> acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl,
> >>> aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl,
> >>> aml:tests/data/acpi/virt/SPCR].
> >>>
> >>> The diff is here:
> >>>
> >>> --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300
> >>> +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300
> >>> @@ -1,57 +1,49 @@
> >>>/*
> >>> * Intel ACPI Component Architecture
> >>> * AML/ASL+ Disassembler version 20220331 (64-bit version)
> >>> * Copyright (c) 2000 - 2022 Intel Corporation
> >>>
> >>> (...)
> >>>
> >>>[000h 4]Signature : "SPCR"[Serial Port
> >>> Console Redirection Table]
> >>> -[004h 0004 4] Table Length : 0050
> >>> +[004h 0004 4] Table Length : 004F
> >>>[008h 0008 1] Revision : 02
> >>> -[009h 0009 1] Checksum : B1
> >>> +[009h 0009 1] Checksum : B2
> >>>[00Ah 0010 6] Oem ID : "BOCHS "
> >>>
> >>> (...)
> >>>
> >>> -[042h 0066 2]PCI Vendor ID :
> >>> +[042h 0066 2]PCI Vendor ID : 00FF
> >>>
> >>>
> >>> After inspecting the common helper and what the original ARM code was
> >>> doing
> >>> I found out that we're missing something down there:
> >>>
> >>>
> >>> On 1/15/24 22:09, Sia Jee Heng wrote:
> RISC-V should also generate the SPCR in a manner similar to ARM.
> Therefore, instead of replicating the code, relocate this function
> to the common AML build.
>
> Signed-off-by: Sia Jee Heng
> ---
> hw/acpi/aml-build.c | 51
> hw/arm/virt-acpi-build.c| 68 +++--
> include/hw/acpi/acpi-defs.h | 33 ++
> include/hw/acpi/aml-build.h | 4 +++
> 4 files changed, 115 insertions(+), 41 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index af66bde0f5..f3904650e4 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray
> *tbl, uint32_t flags,
> }
> }
>
> +void build_spcr(GArray *table_data, BIOSLinker *linker,
> +const AcpiSpcrData *f, const uint8_t rev,
> +const char *oem_id, const char *oem_table_id)
> +{
> +AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> +.oem_table_id = oem_table_id };
> +
> +acpi_table_begin(, table_data);
> +/* Interface type */
> +build_append_int_noprefix(table_data, f->interface_type, 1);
> +/* Reserved */
> +build_append_int_noprefix(table_data, 0, 3);
> +/* Base Address */
> +build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
> + f->base_addr.offset, f->base_addr.size,
> + f->base_addr.addr);
> +/* Interrupt type
Re: [PATCH v5 13/24] tests/avocado: replay_linux.py remove the timeout expected guards
On Wed Mar 20, 2024 at 3:57 AM AEST, Alex Bennée wrote:
> Nicholas Piggin writes:
>
> > replay_linux tests with virtio on aarch64 gciv3 and x86-64 q35 machines
> > seems to be more reliable now, so timeouts are no longer expected.
> > pc_i440fx, gciv2, and non-virtio still have problems, so mark them as
> > flaky: they are not just long-running, but can hang indefinitely.
> >
> > These tests take about 400 seconds each, so add the SPEED=slow guard.
> >
> > Signed-off-by: Nicholas Piggin
> > ---
> > tests/avocado/replay_linux.py | 10 +++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/avocado/replay_linux.py b/tests/avocado/replay_linux.py
> > index b3b91ddd9a..b3b74a367c 100644
> > --- a/tests/avocado/replay_linux.py
> > +++ b/tests/avocado/replay_linux.py
> > @@ -118,7 +118,7 @@ def run_replay_dump(self, replay_path):
> > except subprocess.CalledProcessError:
> > self.fail('replay-dump.py failed')
> >
> > -@skipUnless(os.getenv('AVOCADO_TIMEOUT_EXPECTED'), 'Test might timeout')
> > +@skipUnless(os.getenv('SPEED') == 'slow', 'runtime limited')
> > class ReplayLinuxX8664(ReplayLinux):
> > """
> > :avocado: tags=arch:x86_64
> > @@ -127,19 +127,21 @@ class ReplayLinuxX8664(ReplayLinux):
> >
> > chksum =
> > 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
> >
> > +@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable')
> > def test_pc_i440fx(self):
> > """
> > :avocado: tags=machine:pc
> > """
> > self.run_rr(shift=1)
> >
> > +@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable')
> > def test_pc_q35(self):
> > """
> > :avocado: tags=machine:q35
> > """
> > self.run_rr(shift=3)
> >
> > -@skipUnless(os.getenv('AVOCADO_TIMEOUT_EXPECTED'), 'Test might timeout')
> > +@skipUnless(os.getenv('SPEED') == 'slow', 'runtime limited')
> > class ReplayLinuxX8664Virtio(ReplayLinux):
> > """
> > :avocado: tags=arch:x86_64
> > @@ -153,6 +155,7 @@ class ReplayLinuxX8664Virtio(ReplayLinux):
> >
> > chksum =
> > 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
> >
> > +@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable')
> > def test_pc_i440fx(self):
> > """
> > :avocado: tags=machine:pc
> > @@ -165,7 +168,7 @@ def test_pc_q35(self):
> > """
> > self.run_rr(shift=3)
> >
> > -@skipUnless(os.getenv('AVOCADO_TIMEOUT_EXPECTED'), 'Test might timeout')
> > +@skipUnless(os.getenv('SPEED') == 'slow', 'runtime limited')
> > class ReplayLinuxAarch64(ReplayLinux):
> > """
> > :avocado: tags=accel:tcg
> > @@ -187,6 +190,7 @@ def get_common_args(self):
> > '-device', 'virtio-rng-pci,rng=rng0',
> > '-object', 'rng-builtin,id=rng0')
> >
> > +@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is
> > unstable')
>
> This needs to apply to both I think:
>
> (5/7) ./tests/avocado/replay_linux.py:ReplayLinuxAarch64.test_virt_gicv2:
> SKIP: Test is unstable
> (6/7)
> ./tests/avocado/replay_linux.py:ReplayLinuxAarch64.test_virt_gicv3:
> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
> Timeout reached\nOriginal status: ERROR\n{'name':
> '6-./tests/avocado/replay_linux.py:ReplayLinuxAarch64.test_virt_gicv3',
> 'logdir':
> '/home/alex/avocado/job-results/job-2024-03-19T16.50-686495d/test-results/...
> (1800.17 s)
Oh, aarch64 is hanging for you too? It was passing for me... But I'll
guard it out for now and maybe we can get back to it later.
Thanks,
Nick
>
> With that:
>
> Reviewed-by: Alex Bennée
Re: [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits
On Wed, 2024-03-20 at 14:31 +1000, Nicholas Piggin wrote:
> On Wed Mar 20, 2024 at 11:50 AM AEST, Benjamin Gray wrote:
> > The DEXCR emulation was recently changed to a 32-bit register,
> > possibly
> > because it does have a 32-bit read-only view. It is a full 64-bit
> > SPR though, so use the corresponding 64-bit write functions.
> >
>
> Thanks, paper bag for me.
>
> > Fixes: c9de140c2171 ("target/ppc: Fix width of some 32-bit SPRs")
>
> Should that hash be fbda88f7abdee?
>
Oops, yeah, somehow pasted the local commit hash for this patch itself
> Reviewed-by: Nicholas Piggin
>
> > Signed-off-by: Benjamin Gray
> > ---
> > target/ppc/cpu_init.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > index 7e65f08147..22fdea093b 100644
> > --- a/target/ppc/cpu_init.c
> > +++ b/target/ppc/cpu_init.c
> > @@ -5820,7 +5820,7 @@ static void
> > register_power10_dexcr_sprs(CPUPPCState *env)
> > {
> > spr_register(env, SPR_DEXCR, "DEXCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - _read_generic, _write_generic32,
> > + _read_generic, _write_generic,
> > 0);
> >
> > spr_register(env, SPR_UDEXCR, "UDEXCR",
> > @@ -5831,7 +5831,7 @@ static void
> > register_power10_dexcr_sprs(CPUPPCState *env)
> > spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > SPR_NOACCESS, SPR_NOACCESS,
> > - _read_generic, _write_generic32,
> > + _read_generic, _write_generic,
> > 0);
> >
> > spr_register(env, SPR_UHDEXCR, "UHDEXCR",
>
Re: [PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs
On Wed Mar 20, 2024 at 11:50 AM AEST, Benjamin Gray wrote:
> The GDB server protocol assigns an arbitrary numbering of the SPRs.
> We track this correspondence on each SPR with gdb_id, using it to
> resolve any SPR requests GDB makes.
>
> Early on we generate an XML representation of the SPRs to give GDB,
> including this numbering. However the XML is cached globally, and we
> skip setting the SPR gdb_id values on subsequent threads if we detect
> it is cached. This causes QEMU to fail to resolve SPR requests against
> secondary CPUs because it cannot find the matching gdb_id value on that
> thread's SPRs.
>
> This is a minimal fix to first assign the gdb_id values, then return
> early if the XML is cached. Otherwise we generate the XML using the
> now already initialised gdb_id values.
Reviewed-by: Nicholas Piggin
>
> Fixes: 1b53948ff8f7 ("target/ppc: Use GDBFeature for dynamic XML")
> Signed-off-by: Benjamin Gray
> ---
> target/ppc/gdbstub.c | 31 ---
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 3f1e61bdb7..3b28d4e21c 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -305,14 +305,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
> unsigned int num_regs = 0;
> int i;
>
> -if (pcc->gdb_spr.xml) {
> -return;
> -}
> -
> -gdb_feature_builder_init(, >gdb_spr,
> - "org.qemu.power.spr", "power-spr.xml",
> - cs->gdb_num_regs);
> -
> for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> ppc_spr_t *spr = >spr_cb[i];
>
> @@ -320,9 +312,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
> continue;
> }
>
> -gdb_feature_builder_append_reg(, g_ascii_strdown(spr->name,
> -1),
> - TARGET_LONG_BITS, num_regs,
> - "int", "spr");
> /*
> * GDB identifies registers based on the order they are
> * presented in the XML. These ids will not match QEMU's
> @@ -335,6 +324,26 @@ static void gdb_gen_spr_feature(CPUState *cs)
> num_regs++;
> }
>
> +if (pcc->gdb_spr.xml) {
> +return;
> +}
> +
> +gdb_feature_builder_init(, >gdb_spr,
> + "org.qemu.power.spr", "power-spr.xml",
> + cs->gdb_num_regs);
> +
> +for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> +ppc_spr_t *spr = >spr_cb[i];
> +
> +if (!spr->name) {
> +continue;
> +}
> +
> +gdb_feature_builder_append_reg(, g_ascii_strdown(spr->name,
> -1),
> + TARGET_LONG_BITS, spr->gdb_id,
> + "int", "spr");
> +}
> +
> gdb_feature_builder_end();
> }
> #endif
Re: [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits
On Wed Mar 20, 2024 at 11:50 AM AEST, Benjamin Gray wrote:
> The DEXCR emulation was recently changed to a 32-bit register, possibly
> because it does have a 32-bit read-only view. It is a full 64-bit
> SPR though, so use the corresponding 64-bit write functions.
>
Thanks, paper bag for me.
> Fixes: c9de140c2171 ("target/ppc: Fix width of some 32-bit SPRs")
Should that hash be fbda88f7abdee?
Reviewed-by: Nicholas Piggin
> Signed-off-by: Benjamin Gray
> ---
> target/ppc/cpu_init.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 7e65f08147..22fdea093b 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5820,7 +5820,7 @@ static void register_power10_dexcr_sprs(CPUPPCState
> *env)
> {
> spr_register(env, SPR_DEXCR, "DEXCR",
> SPR_NOACCESS, SPR_NOACCESS,
> -_read_generic, _write_generic32,
> +_read_generic, _write_generic,
> 0);
>
> spr_register(env, SPR_UDEXCR, "UDEXCR",
> @@ -5831,7 +5831,7 @@ static void register_power10_dexcr_sprs(CPUPPCState
> *env)
> spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> SPR_NOACCESS, SPR_NOACCESS,
> SPR_NOACCESS, SPR_NOACCESS,
> -_read_generic, _write_generic32,
> +_read_generic, _write_generic,
> 0);
>
> spr_register(env, SPR_UHDEXCR, "UHDEXCR",
Re: [PATCH] vhost-vdpa: check vhost_vdpa_set_vring_ready() return value
On Mon, Mar 18, 2024 at 4:27 PM Stefano Garzarella wrote:
>
> On Mon, Mar 18, 2024 at 12:31:59PM +0800, Jason Wang wrote:
> >On Fri, Mar 15, 2024 at 4:23 PM Stefano Garzarella
> >wrote:
> >>
> >> On Thu, Mar 14, 2024 at 11:17:01AM +0800, Jason Wang wrote:
> >> >On Wed, Feb 7, 2024 at 5:27 PM Stefano Garzarella
> >> >wrote:
> >> >>
> >> >> vhost_vdpa_set_vring_ready() could already fail, but if Linux's
> >> >> patch [1] will be merged, it may fail with more chance if
> >> >> userspace does not activate virtqueues before DRIVER_OK when
> >> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated.
> >> >
> >> >I wonder what happens if we just leave it as is.
> >>
> >> Are you referring to this patch or the kernel patch?
> >
> >This patch.
> >
> >>
> >> Here I'm just checking the return value of vhost_vdpa_set_vring_ready().
> >> It can return an error also without that kernel patch, so IMHO is
> >> better to check the return value here in QEMU.
> >>
> >> What issue do you see with this patch applied?
> >
> >For the parent which can enable after driver_ok but not advertise it.
>
> But this patch is not changing anything in that sense, it just controls
> the return value of the VHOST_VDPA_SET_VRING_ENABLE ioctl.
>
> Why would QEMU ignore an error if it can't activate vrings?
> If we really want to ignore it we should document it both in QEMU, but
> also in the kernel, because honestly the way the code is now it
> shouldn't fail from what I understand.
>
> That said, even if we ignore it, IMHO we should at least print a warning
> in QEMU.
Right.
>
> >
> >(To say the truth, I'm not sure if we need to care about this)
>
> I agree on that, but this is related to the patch in the kernel, not
.> this simple patch to fix QEMU error path, right?
Or it's the charge of the Qemu vDPA layer to avoid calling
set_vq_ready() after driver_ok if no
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK. Or it might be too late.
>
> >
> >>
> >> >
> >> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We do know enabling could be
> >> >done after driver_ok.
> >> >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK: We don't know whether
> >> >enabling could be done after driver_ok or not.
> >>
> >> I see your point, indeed I didn't send a v2 of that patch.
> >> Maybe we should document that, because it could be interpreted that if
> >> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not negotiated the enabling
> >> should always be done before driver_ok (which is true for example in
> >> VDUSE).
> >
> >I see, so I think we probably need the fix.
> >
> >>
> >> BTW I think we should discuss it in the kernel patch.
> >>
> >> Thanks,
> >> Stefano
> >>
> >> >
> >> >Thanks
> >> >
> >> >>
> >> >> So better check its return value anyway.
> >> >>
> >> >> [1]
> >> >> https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarz...@redhat.com/T/#u
> >> >>
> >> >> Signed-off-by: Stefano Garzarella
> >> >> ---
> >> >> Note: This patch conflicts with [2], but the resolution is simple,
> >> >> so for now I sent a patch for the current master, but I'll rebase
> >> >> this patch if we merge the other one first.
> >
> >Will go through [2].
>
> Here I meant that the conflict is only in the code touched, because
> Kevin's patch remove/move some of the code touched by this patch.
> And rightly he checked the return value of the ioctl as I would like to
> do in the other places where we call the same ioctl.
>
> So honestly I still don't understand what's wrong with this patch...
Nothing wrong now.
Acked-by: Jason Wang
Thanks
>
> Thanks,
> Stefano
>
> >
> >Thanks
> >
> >
> >> >>
> >> >> [2]
> >> >> https://lore.kernel.org/qemu-devel/20240202132521.32714-1-kw...@redhat.com/
> >> >> ---
> >> >> hw/virtio/vdpa-dev.c | 8 +++-
> >> >> net/vhost-vdpa.c | 15 ---
> >> >> 2 files changed, 19 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> >> >> index eb9ecea83b..d57cd76c18 100644
> >> >> --- a/hw/virtio/vdpa-dev.c
> >> >> +++ b/hw/virtio/vdpa-dev.c
> >> >> @@ -259,7 +259,11 @@ static int vhost_vdpa_device_start(VirtIODevice
> >> >> *vdev, Error **errp)
> >> >> goto err_guest_notifiers;
> >> >> }
> >> >> for (i = 0; i < s->dev.nvqs; ++i) {
> >> >> -vhost_vdpa_set_vring_ready(>vdpa, i);
> >> >> +ret = vhost_vdpa_set_vring_ready(>vdpa, i);
> >> >> +if (ret < 0) {
> >> >> +error_setg_errno(errp, -ret, "Error starting vring %d", i);
> >> >> +goto err_dev_stop;
> >> >> +}
> >> >> }
> >> >> s->started = true;
> >> >>
> >> >> @@ -274,6 +278,8 @@ static int vhost_vdpa_device_start(VirtIODevice
> >> >> *vdev, Error **errp)
> >> >>
> >> >> return ret;
> >> >>
> >> >> +err_dev_stop:
> >> >> +vhost_dev_stop(>dev, vdev, false);
> >> >> err_guest_notifiers:
> >> >> k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> >> >> err_host_notifiers:
> >> >> diff --git a/net/vhost-vdpa.c
[PATCH] ui/console: initialize QemuDmaBuf from ui/console.
From: Dongwon Kim
It is safer to create, initialize, and access all the parameters
in QemuDmaBuf from a central location, ui/console, instead of
hw/virtio-gpu or hw/vfio modules.
Cc: Marc-André Lureau
Cc: Vivek Kasireddy
Signed-off-by: Dongwon Kim
---
hw/display/virtio-gpu-udmabuf.c | 27 +++-
hw/vfio/display.c | 35 -
include/hw/vfio/vfio-common.h | 2 +-
include/hw/virtio/virtio-gpu.h | 2 +-
include/ui/console.h| 10 ++
ui/console.c| 55 +
6 files changed, 98 insertions(+), 33 deletions(-)
diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index d51184d658..dde6c8e9d9 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -162,7 +162,7 @@ static void virtio_gpu_free_dmabuf(VirtIOGPU *g, VGPUDMABuf
*dmabuf)
struct virtio_gpu_scanout *scanout;
scanout = >parent_obj.scanout[dmabuf->scanout_id];
-dpy_gl_release_dmabuf(scanout->con, >buf);
+dpy_gl_release_dmabuf(scanout->con, dmabuf->buf);
QTAILQ_REMOVE(>dmabuf.bufs, dmabuf, next);
g_free(dmabuf);
}
@@ -181,17 +181,10 @@ static VGPUDMABuf
}
dmabuf = g_new0(VGPUDMABuf, 1);
-dmabuf->buf.width = r->width;
-dmabuf->buf.height = r->height;
-dmabuf->buf.stride = fb->stride;
-dmabuf->buf.x = r->x;
-dmabuf->buf.y = r->y;
-dmabuf->buf.backing_width = fb->width;
-dmabuf->buf.backing_height = fb->height;
-dmabuf->buf.fourcc = qemu_pixman_to_drm_format(fb->format);
-dmabuf->buf.fd = res->dmabuf_fd;
-dmabuf->buf.allow_fences = true;
-dmabuf->buf.draw_submitted = false;
+dmabuf->buf = dpy_gl_create_dmabuf(r->width, r->height, fb->stride,
+ r->x, r->y, fb->width, fb->height,
+ qemu_pixman_to_drm_format(fb->format),
+ 0, res->dmabuf_fd, false);
dmabuf->scanout_id = scanout_id;
QTAILQ_INSERT_HEAD(>dmabuf.bufs, dmabuf, next);
@@ -206,21 +199,23 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
{
struct virtio_gpu_scanout *scanout = >parent_obj.scanout[scanout_id];
VGPUDMABuf *new_primary, *old_primary = NULL;
+uint32_t width, height;
new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb, r);
if (!new_primary) {
return -EINVAL;
}
+width = dpy_gl_dmabuf_get_width(new_primary->buf);
+height = dpy_gl_dmabuf_get_height(new_primary->buf);
+
if (g->dmabuf.primary[scanout_id]) {
old_primary = g->dmabuf.primary[scanout_id];
}
g->dmabuf.primary[scanout_id] = new_primary;
-qemu_console_resize(scanout->con,
-new_primary->buf.width,
-new_primary->buf.height);
-dpy_gl_scanout_dmabuf(scanout->con, _primary->buf);
+qemu_console_resize(scanout->con, width, height);
+dpy_gl_scanout_dmabuf(scanout->con, new_primary->buf);
if (old_primary) {
virtio_gpu_free_dmabuf(g, old_primary);
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 1aa440c663..a3bdb01789 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -241,14 +241,10 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice
*vdev,
dmabuf = g_new0(VFIODMABuf, 1);
dmabuf->dmabuf_id = plane.dmabuf_id;
-dmabuf->buf.width = plane.width;
-dmabuf->buf.height = plane.height;
-dmabuf->buf.backing_width = plane.width;
-dmabuf->buf.backing_height = plane.height;
-dmabuf->buf.stride = plane.stride;
-dmabuf->buf.fourcc = plane.drm_format;
-dmabuf->buf.modifier = plane.drm_format_mod;
-dmabuf->buf.fd = fd;
+dmabuf->buf = dpy_gl_create_dmabuf(plane.width, plane.height, plane.stride,
+ 0, 0, plane.width, plane.height,
+ plane.drm_format, plane.drm_format_mod,
+ fd, false);
if (plane_type == DRM_PLANE_TYPE_CURSOR) {
vfio_display_update_cursor(dmabuf, );
}
@@ -259,9 +255,15 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice
*vdev,
static void vfio_display_free_one_dmabuf(VFIODisplay *dpy, VFIODMABuf *dmabuf)
{
+int fd;
+
QTAILQ_REMOVE(>dmabuf.bufs, dmabuf, next);
-dpy_gl_release_dmabuf(dpy->con, >buf);
-close(dmabuf->buf.fd);
+fd = dpy_gl_dmabuf_get_fd(dmabuf->buf);
+if (fd > -1) {
+close(fd);
+}
+
+dpy_gl_release_dmabuf(dpy->con, dmabuf->buf);
g_free(dmabuf);
}
@@ -286,6 +288,7 @@ static void vfio_display_dmabuf_update(void *opaque)
VFIOPCIDevice *vdev = opaque;
VFIODisplay *dpy = vdev->dpy;
VFIODMABuf *primary, *cursor;
+uint32_t width, height;
bool free_bufs = false, new_cursor = false;
primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
@@ -296,11 +299,13 @@ static void
Re: Pending network patches
On Wed, Mar 20, 2024 at 11:33 AM Akihiko Odaki wrote:
>
> Hi Jason,
>
> I have this and a few other network-related patches not reviewed. Can
> you review them?
> I have the following patches ready for review:
>
> https://patchew.org/QEMU/20240212-tap-v2-1-94e2ee18b...@daynix.com/
> ("[PATCH v2] tap-win32: Remove unnecessary stubs")
>
> https://patchew.org/QEMU/20230921094851.36295-1-akihiko.od...@daynix.com/
> ("[PATCH v2] Revert "tap: setting error appropriately when calling
> net_init_tap_one()"")
>
> https://patchew.org/QEMU/20231219-glib-v1-0-1b040d286...@daynix.com/
> ("[PATCH 0/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()")
>
> Regards,
> Akihiko Odaki
Will do.
Thanks
>
Pending network patches
Hi Jason,
I have this and a few other network-related patches not reviewed. Can
you review them?
I have the following patches ready for review:
https://patchew.org/QEMU/20240212-tap-v2-1-94e2ee18b...@daynix.com/
("[PATCH v2] tap-win32: Remove unnecessary stubs")
https://patchew.org/QEMU/20230921094851.36295-1-akihiko.od...@daynix.com/
("[PATCH v2] Revert "tap: setting error appropriately when calling
net_init_tap_one()"")
https://patchew.org/QEMU/20231219-glib-v1-0-1b040d286...@daynix.com/
("[PATCH 0/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()")
Regards,
Akihiko Odaki
Re: [PATCH v4 2/2] vhost: Perform memory section dirty scans once per iteration
On Tue, Mar 19, 2024 at 6:16 AM Si-Wei Liu wrote:
>
>
>
> On 3/17/2024 8:22 PM, Jason Wang wrote:
> > On Sat, Mar 16, 2024 at 2:45 AM Si-Wei Liu wrote:
> >>
> >>
> >> On 3/14/2024 9:03 PM, Jason Wang wrote:
> >>> On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu wrote:
> On setups with one or more virtio-net devices with vhost on,
> dirty tracking iteration increases cost the bigger the number
> amount of queues are set up e.g. on idle guests migration the
> following is observed with virtio-net with vhost=on:
>
> 48 queues -> 78.11% [.] vhost_dev_sync_region.isra.13
> 8 queues -> 40.50% [.] vhost_dev_sync_region.isra.13
> 1 queue -> 6.89% [.] vhost_dev_sync_region.isra.13
> 2 devices, 1 queue -> 18.60% [.] vhost_dev_sync_region.isra.14
>
> With high memory rates the symptom is lack of convergence as soon
> as it has a vhost device with a sufficiently high number of queues,
> the sufficient number of vhost devices.
>
> On every migration iteration (every 100msecs) it will redundantly
> query the *shared log* the number of queues configured with vhost
> that exist in the guest. For the virtqueue data, this is necessary,
> but not for the memory sections which are the same. So essentially
> we end up scanning the dirty log too often.
>
> To fix that, select a vhost device responsible for scanning the
> log with regards to memory sections dirty tracking. It is selected
> when we enable the logger (during migration) and cleared when we
> disable the logger. If the vhost logger device goes away for some
> reason, the logger will be re-selected from the rest of vhost
> devices.
>
> After making mem-section logger a singleton instance, constant cost
> of 7%-9% (like the 1 queue report) will be seen, no matter how many
> queues or how many vhost devices are configured:
>
> 48 queues -> 8.71%[.] vhost_dev_sync_region.isra.13
> 2 devices, 8 queues -> 7.97% [.] vhost_dev_sync_region.isra.14
>
> Co-developed-by: Joao Martins
> Signed-off-by: Joao Martins
> Signed-off-by: Si-Wei Liu
>
> ---
> v3 -> v4:
> - add comment to clarify effect on cache locality and
> performance
>
> v2 -> v3:
> - add after-fix benchmark to commit log
> - rename vhost_log_dev_enabled to vhost_dev_should_log
> - remove unneeded comparisons for backend_type
> - use QLIST array instead of single flat list to store vhost
> logger devices
> - simplify logger election logic
> ---
> hw/virtio/vhost.c | 67
> ++-
> include/hw/virtio/vhost.h | 1 +
> 2 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 612f4db..58522f1 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -45,6 +45,7 @@
>
> static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX];
> static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX];
> +static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX];
>
> /* Memslots used by backends that support private memslots (without
> an fd). */
> static unsigned int used_memslots;
> @@ -149,6 +150,47 @@ bool vhost_dev_has_iommu(struct vhost_dev *dev)
> }
> }
>
> +static inline bool vhost_dev_should_log(struct vhost_dev *dev)
> +{
> +assert(dev->vhost_ops);
> +assert(dev->vhost_ops->backend_type > VHOST_BACKEND_TYPE_NONE);
> +assert(dev->vhost_ops->backend_type < VHOST_BACKEND_TYPE_MAX);
> +
> +return dev ==
> QLIST_FIRST(_log_devs[dev->vhost_ops->backend_type]);
> >>> A dumb question, why not simple check
> >>>
> >>> dev->log == vhost_log_shm[dev->vhost_ops->backend_type]
> >> Because we are not sure if the logger comes from vhost_log_shm[] or
> >> vhost_log[]. Don't want to complicate the check here by calling into
> >> vhost_dev_log_is_shared() everytime when the .log_sync() is called.
> > It has very low overhead, isn't it?
> Whether this has low overhead will have to depend on the specific
> backend's implementation for .vhost_requires_shm_log(), which the common
> vhost layer should not assume upon or rely on the current implementation.
>
> >
> > static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> > {
> > return dev->vhost_ops->vhost_requires_shm_log &&
> > dev->vhost_ops->vhost_requires_shm_log(dev);
> > }
For example, if I understand the code correctly, the log type won't be
changed during runtime, so we can endup with a boolean to record that
instead of a query ops?
> >
> > And it helps to simplify the logic.
> Generally yes, but when it comes to hot path operations the
Re: [PATCH v4 1/2] vhost: dirty log should be per backend type
On Tue, Mar 19, 2024 at 6:06 AM Si-Wei Liu wrote:
>
>
>
> On 3/17/2024 8:20 PM, Jason Wang wrote:
> > On Sat, Mar 16, 2024 at 2:33 AM Si-Wei Liu wrote:
> >>
> >>
> >> On 3/14/2024 8:50 PM, Jason Wang wrote:
> >>> On Fri, Mar 15, 2024 at 5:39 AM Si-Wei Liu wrote:
> There could be a mix of both vhost-user and vhost-kernel clients
> in the same QEMU process, where separate vhost loggers for the
> specific vhost type have to be used. Make the vhost logger per
> backend type, and have them properly reference counted.
> >>> It's better to describe what's the advantage of doing this.
> >> Yes, I can add that to the log. Although it's a niche use case, it was
> >> actually a long standing limitation / bug that vhost-user and
> >> vhost-kernel loggers can't co-exist per QEMU process, but today it's
> >> just silent failure that may be ended up with. This bug fix removes that
> >> implicit limitation in the code.
> > Ok.
> >
> Suggested-by: Michael S. Tsirkin
> Signed-off-by: Si-Wei Liu
>
> ---
> v3->v4:
> - remove checking NULL return value from vhost_log_get
>
> v2->v3:
> - remove non-effective assertion that never be reached
> - do not return NULL from vhost_log_get()
> - add neccessary assertions to vhost_log_get()
> ---
> hw/virtio/vhost.c | 45 +
> 1 file changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 2c9ac79..612f4db 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -43,8 +43,8 @@
> do { } while (0)
> #endif
>
> -static struct vhost_log *vhost_log;
> -static struct vhost_log *vhost_log_shm;
> +static struct vhost_log *vhost_log[VHOST_BACKEND_TYPE_MAX];
> +static struct vhost_log *vhost_log_shm[VHOST_BACKEND_TYPE_MAX];
>
> /* Memslots used by backends that support private memslots (without
> an fd). */
> static unsigned int used_memslots;
> @@ -287,6 +287,10 @@ static int vhost_set_backend_type(struct vhost_dev
> *dev,
> r = -1;
> }
>
> +if (r == 0) {
> +assert(dev->vhost_ops->backend_type == backend_type);
> +}
> +
> >>> Under which condition could we hit this?
> >> Just in case some other function inadvertently corrupted this earlier,
> >> we have to capture discrepancy in the first place... On the other hand,
> >> it will be helpful for other vhost backend writers to diagnose day-one
> >> bug in the code. I feel just code comment here will not be
> >> sufficient/helpful.
> > See below.
> >
> >>>It seems not good to assert a local logic.
> >> It seems to me quite a few local asserts are in the same file already,
> >> vhost_save_backend_state,
> > For example it has assert for
> >
> > assert(!dev->started);
> >
> > which is not the logic of the function itself but require
> > vhost_dev_start() not to be called before.
> >
> > But it looks like this patch you assert the code just a few lines
> > above the assert itself?
> Yes, that was the intent - for e.g. xxx_ops may contain corrupted
> xxx_ops.backend_type already before coming to this
> vhost_set_backend_type() function. And we may capture this corrupted
> state by asserting the expected xxx_ops.backend_type (to be consistent
> with the backend_type passed in),
This can happen for all variables. Not sure why backend_ops is special.
> which needs be done in the first place
> when this discrepancy is detected. In practice I think there should be
> no harm to add this assert, but this will add warranted guarantee to the
> current code.
For example, such corruption can happen after the assert() so a TOCTOU issue.
Thanks
>
> Regards,
> -Siwei
>
> >
> > dev->vhost_ops = _ops;
> >
> > ...
> >
> > assert(dev->vhost_ops->backend_type == backend_type)
> >
> > ?
> >
> > Thanks
> >
> >> vhost_load_backend_state,
> >> vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local
> >> assert a problem?
> >>
> >> Thanks,
> >> -Siwei
> >>
> >>> Thanks
> >>>
>
Re: [RFC PATCH v8 05/23] target/arm: Support MSR access to ALLINT
On 2024/3/20 1:30, Peter Maydell wrote:
> On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan wrote:
>>
>> Support ALLINT msr access as follow:
>> mrs , ALLINT// read allint
>> msr ALLINT, // write allint with imm
>>
>> Signed-off-by: Jinjie Ruan
>> Reviewed-by: Richard Henderson
>> ---
>> v5:
>> - Add Reviewed-by.
>> v4:
>> - Remove arm_is_el2_enabled() check in allint_check().
>> - Change to env->pstate instead of env->allint.
>> v3:
>> - Remove EL0 check in aa64_allint_access() which alreay checks in .access
>> PL1_RW.
>> - Use arm_hcrx_el2_eff() in aa64_allint_access() instead of
>> env->cp15.hcrx_el2.
>> - Make ALLINT msr access function controlled by aa64_nmi.
>> ---
>> target/arm/helper.c | 34 ++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index b19a0178ce..aa0151c775 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -4752,6 +4752,36 @@ static void aa64_daif_write(CPUARMState *env, const
>> ARMCPRegInfo *ri,
>> env->daif = value & PSTATE_DAIF;
>> }
>>
>> +static void aa64_allint_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> + uint64_t value)
>> +{
>> +env->pstate = (env->pstate & ~PSTATE_ALLINT) | (value & PSTATE_ALLINT);
>> +}
>> +
>> +static uint64_t aa64_allint_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +return env->pstate & PSTATE_ALLINT;
>> +}
>> +
>> +static CPAccessResult aa64_allint_access(CPUARMState *env,
>> + const ARMCPRegInfo *ri, bool
>> isread)
>> +{
>> +if (arm_current_el(env) == 1 && (arm_hcrx_el2_eff(env) & HCRX_TALLINT))
>> {
>> +return CP_ACCESS_TRAP_EL2;
>> +}
>> +return CP_ACCESS_OK;
>
> Forgot to note in my earlier email: HCRX_EL2.TALLINT traps
> only writes to ALLINT, not reads, so the condition
> here needs to also look at 'isread'.
Thank you! I'll fix it.
>
>> +}
>
> thanks
> -- PMM
Re: [PATCH v2 0/4] ui/console: Remove console_select()
On 2024/03/19 17:29, Marc-André Lureau wrote: Hi Akihiko On Tue, Mar 19, 2024 at 7:09 AM Akihiko Odaki wrote: ui/console has a concept of "active" console; the active console is used when NULL is set for DisplayListener::con, and console_select() updates the active console state. However, the global nature of the state cause odd behaviors, and replacing NULL with the active console also resulted in extra code. Remove it to solve these problems. The active console state is shared, so if there are two displays referring to the active console, switching the console for one will also affect the other. All displays that use the active console state, namely cocoa, curses, and vnc, need to reset some of its state before switching the console, and such a reset operation cannot be performed if the console is switched by another display. This can result in stuck keys, for example. While the active console state is shared, displays other than cocoa, curses, and vnc don't update the state. A chardev-vc inherits the size of the active console, but it does not make sense for such a display. This series removes the shared "active" console state from ui/console. curses, cocoa, and vnc will hold the reference to the console currently shown with DisplayListener::con. This also eliminates the need to replace NULL with the active console and save code. Signed-off-by: Akihiko Odaki lgtm Reviewed-by: Marc-André Lureau I am willing to take that for 9.0. Is there any bug already opened about the issues it solves? No, I'm not aware of one.
[ANNOUNCE] QEMU 9.0.0-rc0 is now available
Hello, On behalf of the QEMU Team, I'd like to announce the availability of the first release candidate for the QEMU 9.0 release. This release is meant for testing purposes and should not be used in a production environment. http://download.qemu.org/qemu-9.0.0-rc0.tar.xz http://download.qemu.org/qemu-9.0.0-rc0.tar.xz.sig You can help improve the quality of the QEMU 9.0 release by testing this release and reporting bugs using our GitLab issue tracker: https://gitlab.com/qemu-project/qemu/-/milestones/11 The release plan, as well a documented known issues for release candidates, are available at: http://wiki.qemu.org/Planning/9.0 Please add entries to the ChangeLog for the 9.0 release below: http://wiki.qemu.org/ChangeLog/9.0 Thank you to everyone involved!
Re: [RFC PATCH v8 05/23] target/arm: Support MSR access to ALLINT
On 2024/3/20 0:45, Peter Maydell wrote:
> On Mon, 18 Mar 2024 at 09:37, Jinjie Ruan wrote:
>>
>> Support ALLINT msr access as follow:
>> mrs , ALLINT// read allint
>> msr ALLINT, // write allint with imm
>>
>> Signed-off-by: Jinjie Ruan
>> Reviewed-by: Richard Henderson
>> ---
>> v5:
>> - Add Reviewed-by.
>> v4:
>> - Remove arm_is_el2_enabled() check in allint_check().
>> - Change to env->pstate instead of env->allint.
>> v3:
>> - Remove EL0 check in aa64_allint_access() which alreay checks in .access
>> PL1_RW.
>> - Use arm_hcrx_el2_eff() in aa64_allint_access() instead of
>> env->cp15.hcrx_el2.
>> - Make ALLINT msr access function controlled by aa64_nmi.
>> ---
>> target/arm/helper.c | 34 ++
>> 1 file changed, 34 insertions(+)
>
> If you configure with --target-list=aarch64-softmmu,arm-softmmu
> you'll find this fails to build:
>
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index b19a0178ce..aa0151c775 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -4752,6 +4752,36 @@ static void aa64_daif_write(CPUARMState *env, const
>> ARMCPRegInfo *ri,
>> env->daif = value & PSTATE_DAIF;
>> }
>>
>> +static void aa64_allint_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> + uint64_t value)
>> +{
>> +env->pstate = (env->pstate & ~PSTATE_ALLINT) | (value & PSTATE_ALLINT);
>> +}
>> +
>> +static uint64_t aa64_allint_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +return env->pstate & PSTATE_ALLINT;
>> +}
>> +
>> +static CPAccessResult aa64_allint_access(CPUARMState *env,
>> + const ARMCPRegInfo *ri, bool
>> isread)
>> +{
>> +if (arm_current_el(env) == 1 && (arm_hcrx_el2_eff(env) & HCRX_TALLINT))
>> {
>> +return CP_ACCESS_TRAP_EL2;
>> +}
>> +return CP_ACCESS_OK;
>> +}
>> +
>> +static const ARMCPRegInfo nmi_reginfo[] = {
>> +{ .name = "ALLINT", .state = ARM_CP_STATE_AA64,
>> + .opc0 = 3, .opc1 = 0, .opc2 = 0, .crn = 4, .crm = 3,
>> + .type = ARM_CP_NO_RAW,
>> + .access = PL1_RW, .accessfn = aa64_allint_access,
>> + .fieldoffset = offsetof(CPUARMState, pstate),
>> + .writefn = aa64_allint_write, .readfn = aa64_allint_read,
>> + .resetfn = arm_cp_reset_ignore },
>> +};
>
> These functions and the array have been put in a bit of the
> file that is built whether TARGET_AARCH64 is defined or not...
>
>> +
>> static uint64_t aa64_pan_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> {
>> return env->pstate & PSTATE_PAN;
>> @@ -9889,6 +9919,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>> if (cpu_isar_feature(aa64_nv2, cpu)) {
>> define_arm_cp_regs(cpu, nv2_reginfo);
>> }
>> +
>> +if (cpu_isar_feature(aa64_nmi, cpu)) {
>> +define_arm_cp_regs(cpu, nmi_reginfo);
>> +}
>> #endif
>
> ...but the only reference to them is inside an ifdef TARGET_AARCH64.
>
> Moving the nmi_reginfo[] and the functions so they are next to
> some other TARGET_AARCH64-only reginfo array inside one of the
> existing ifdef blocks is probably the nicest fix.
Thank you! I'll fix it.
>
>>
>> if (cpu_isar_feature(any_predinv, cpu)) {
>> --
>
> thanks
> -- PMM
How to compile QEMU with glib source code?
How to compile QEMU with glib source code? But not with the glib library I want to debug QEMU by stepping into glib internally. Thanks.
Re: [PATCH v4 1/2] kvm: add support for guest physical bits
On 3/18/2024 11:53 PM, Gerd Hoffmann wrote:
Query kvm for supported guest physical address bits, in cpuid
function 8008, eax[23:16]. Usually this is identical to host
physical address bits. With NPT or EPT being used this might be
restricted to 48 (max 4-level paging address space size) even if
the host cpu supports more physical address bits.
When set pass this to the guest, using cpuid too. Guest firmware
can use this to figure how big the usable guest physical address
space is, so PCI bar mapping are actually reachable.
Signed-off-by: Gerd Hoffmann
---
target/i386/cpu.h | 1 +
target/i386/cpu.c | 1 +
target/i386/kvm/kvm-cpu.c | 31 ++-
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 952174bb6f52..d427218827f6 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2026,6 +2026,7 @@ struct ArchCPU {
/* Number of physical address bits supported */
uint32_t phys_bits;
+uint32_t guest_phys_bits;
/* in order to simplify APIC support, we leave this pointer to the
user */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9a210d8d9290..c88c895a5b3e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6570,6 +6570,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
uint32_t count,
if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
/* 64 bit processor */
*eax |= (cpu_x86_virtual_addr_width(env) << 8);
+ *eax |= (cpu->guest_phys_bits << 16);
}
*ebx = env->features[FEAT_8000_0008_EBX];
if (cs->nr_cores * cs->nr_threads > 1) {
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 9c791b7b0520..5132bb96abd5 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -18,10 +18,33 @@
#include "kvm_i386.h"
#include "hw/core/accel-cpu.h"
+static void kvm_set_guest_phys_bits(CPUState *cs)
+{
+X86CPU *cpu = X86_CPU(cs);
+uint32_t eax, guest_phys_bits;
+
+eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x8008, 0, R_EAX);
+guest_phys_bits = (eax >> 16) & 0xff;
+if (!guest_phys_bits) {
+return;
+}
+
+if (cpu->guest_phys_bits == 0 ||
+cpu->guest_phys_bits > guest_phys_bits) {
+cpu->guest_phys_bits = guest_phys_bits;
+}
+
+if (cpu->host_phys_bits_limit &&
+cpu->guest_phys_bits > cpu->host_phys_bits_limit) {
+cpu->guest_phys_bits = cpu->host_phys_bits_limit;
host_phys_bits_limit takes effect only when cpu->host_phys_bits is set.
If users pass configuration like "-cpu
qemu64,phys-bits=52,host-phys-bits-limit=45", the cpu->guest_phys_bits
will be set to 45. I think this is not what we want, though the usage
seems insane.
We can guard it as
if (cpu->host_phys_bits && cpu->host_phys_bits_limit &&
cpu->guest_phys_bits > cpu->host_phys_bits_limt)
{
}
Simpler, we can guard with cpu->phys_bits like below, because
cpu->host_phys_bits_limit is used to guard cpu->phys_bits in
host_cpu_realizefn()
if (cpu->guest_phys_bits > cpu->phys_bits) {
cpu->guest_phys_bits = cpu->phys_bits;
}
with this resolved,
Reviewed-by: Xiaoyao Li
+}
+}
+
static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
{
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = >env;
+bool ret;
/*
* The realize order is important, since x86_cpu_realize() checks if
@@ -50,7 +73,13 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
MSR_IA32_UCODE_REV);
}
}
-return host_cpu_realizefn(cs, errp);
+ret = host_cpu_realizefn(cs, errp);
+if (!ret) {
+return ret;
+}
+
+kvm_set_guest_phys_bits(cs);
+return true;
}
static bool lmce_supported(void)
Re: [PATCH 2/4] target/riscv: Add right functions to set agnostic elements
I will rewrite the patch, and send a new version soon.
Thanks,
Huang Tao
On 2024/3/20 07:32, Richard Henderson wrote:
On 3/19/24 11:57, Daniel Henrique Barboza wrote:
This seems correct but a bit over complicated at first glance. I
wonder if we have
something simpler already done somewhere.
Richard, does ARM (or any other arch) do anything of the sort? Aside
from more trivial
byte swaps using bswap64() I didn't find anything similar.
No, nothing quite like.
We recently posted a big endian related fix here:
[PATCH for 9.0 v15 03/10] target/riscv/vector_helper.c: fix 'vmvr_v'
memcpy endianess
But not sure how to apply it here.
It's almost exactly the same, only with memset instead of memcpy.
if (HOST_BIG_ENDIAN && idx % 8 != 0) {
uint32_t j = ROUND_UP(idx, 8);
memset(vd + H(j - 1), -1, j - idx);
idx = j;
}
memset(vd + idx, -1, tot - idx);
I'll note that you don't need to change the api of vext_set_elems_1s
-- so most of these patches are not required.
r~
[PULL 2/3] target/loongarch: Fix tlb huge page loading issue
From: Xianglai Li
When we use qemu tcg simulation, the page size of bios is 4KB.
When using the level 2 super huge page (page size is 1G) to create the page
table,
it is found that the content of the corresponding address space is abnormal,
resulting in the bios can not start the operating system and graphical
interface normally.
The lddir and ldpte instruction emulation has
a problem with the use of super huge page processing above level 2.
The page size is not correctly calculated,
resulting in the wrong page size of the table entry found by tlb.
Signed-off-by: Xianglai Li
Reviewed-by: Richard Henderson
Signed-off-by: Song Gao
Message-Id: <20240318070332.1273939-1-lixiang...@loongson.cn>
---
target/loongarch/cpu-csr.h| 3 +
target/loongarch/internals.h | 5 --
target/loongarch/tcg/tlb_helper.c | 113 +-
3 files changed, 82 insertions(+), 39 deletions(-)
diff --git a/target/loongarch/cpu-csr.h b/target/loongarch/cpu-csr.h
index c59d7a9fcb..0834e91f30 100644
--- a/target/loongarch/cpu-csr.h
+++ b/target/loongarch/cpu-csr.h
@@ -67,6 +67,9 @@ FIELD(TLBENTRY, D, 1, 1)
FIELD(TLBENTRY, PLV, 2, 2)
FIELD(TLBENTRY, MAT, 4, 2)
FIELD(TLBENTRY, G, 6, 1)
+FIELD(TLBENTRY, HUGE, 6, 1)
+FIELD(TLBENTRY, HGLOBAL, 12, 1)
+FIELD(TLBENTRY, LEVEL, 13, 2)
FIELD(TLBENTRY_32, PPN, 8, 24)
FIELD(TLBENTRY_64, PPN, 12, 36)
FIELD(TLBENTRY_64, NR, 61, 1)
diff --git a/target/loongarch/internals.h b/target/loongarch/internals.h
index a2fc54c8a7..944153b180 100644
--- a/target/loongarch/internals.h
+++ b/target/loongarch/internals.h
@@ -16,11 +16,6 @@
#define TARGET_PHYS_MASK MAKE_64BIT_MASK(0, TARGET_PHYS_ADDR_SPACE_BITS)
#define TARGET_VIRT_MASK MAKE_64BIT_MASK(0, TARGET_VIRT_ADDR_SPACE_BITS)
-/* Global bit used for lddir/ldpte */
-#define LOONGARCH_PAGE_HUGE_SHIFT 6
-/* Global bit for huge page */
-#define LOONGARCH_HGLOBAL_SHIFT 12
-
void loongarch_translate_init(void);
void loongarch_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
diff --git a/target/loongarch/tcg/tlb_helper.c
b/target/loongarch/tcg/tlb_helper.c
index 22be031ac7..57f5308632 100644
--- a/target/loongarch/tcg/tlb_helper.c
+++ b/target/loongarch/tcg/tlb_helper.c
@@ -17,6 +17,34 @@
#include "exec/log.h"
#include "cpu-csr.h"
+static void get_dir_base_width(CPULoongArchState *env, uint64_t *dir_base,
+ uint64_t *dir_width, target_ulong level)
+{
+switch (level) {
+case 1:
+*dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_BASE);
+*dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_WIDTH);
+break;
+case 2:
+*dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_BASE);
+*dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_WIDTH);
+break;
+case 3:
+*dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_BASE);
+*dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_WIDTH);
+break;
+case 4:
+*dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_BASE);
+*dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_WIDTH);
+break;
+default:
+/* level may be zero for ldpte */
+*dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTBASE);
+*dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTWIDTH);
+break;
+}
+}
+
static void raise_mmu_exception(CPULoongArchState *env, target_ulong address,
MMUAccessType access_type, int tlb_error)
{
@@ -485,7 +513,25 @@ target_ulong helper_lddir(CPULoongArchState *env,
target_ulong base,
target_ulong badvaddr, index, phys, ret;
int shift;
uint64_t dir_base, dir_width;
-bool huge = (base >> LOONGARCH_PAGE_HUGE_SHIFT) & 0x1;
+
+if (unlikely((level == 0) || (level > 4))) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "Attepted LDDIR with level %"PRId64"\n", level);
+return base;
+}
+
+if (FIELD_EX64(base, TLBENTRY, HUGE)) {
+if (unlikely(level == 4)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "Attempted use of level 4 huge page\n");
+}
+
+if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
+return base;
+} else {
+return FIELD_DP64(base, TLBENTRY, LEVEL, level);
+}
+}
badvaddr = env->CSR_TLBRBADV;
base = base & TARGET_PHYS_MASK;
@@ -494,30 +540,7 @@ target_ulong helper_lddir(CPULoongArchState *env,
target_ulong base,
shift = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTEWIDTH);
shift = (shift + 1) * 3;
-if (huge) {
-return base;
-}
-switch (level) {
-case 1:
-dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_BASE);
-dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_WIDTH);
-break;
-case 2:
-dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_BASE);
-dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_WIDTH);
-break;
-
[PULL 3/3] target/loongarch: Fix qemu-loongarch64 hang when executing 'll.d $t0, $t0, 0'
On gen_ll, if a->imm is zero, make_address_x return src1,
but the load to destination may clobber src1. We use a new
destination to fix this problem.
Fixes: c5af6628f4be (target/loongarch: Extract make_address_i() helper)
Reviewed-by: Richard Henderson
Suggested-by: Richard Henderson
Signed-off-by: Song Gao
Message-Id: <20240320013955.1561311-1-gaos...@loongson.cn>
---
target/loongarch/tcg/insn_trans/trans_atomic.c.inc | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
b/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
index 80c2e286fd..974bc2a70f 100644
--- a/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
+++ b/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
@@ -5,14 +5,14 @@
static bool gen_ll(DisasContext *ctx, arg_rr_i *a, MemOp mop)
{
-TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
+TCGv t1 = tcg_temp_new();
TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
TCGv t0 = make_address_i(ctx, src1, a->imm);
-tcg_gen_qemu_ld_i64(dest, t0, ctx->mem_idx, mop);
+tcg_gen_qemu_ld_i64(t1, t0, ctx->mem_idx, mop);
tcg_gen_st_tl(t0, tcg_env, offsetof(CPULoongArchState, lladdr));
-tcg_gen_st_tl(dest, tcg_env, offsetof(CPULoongArchState, llval));
-gen_set_gpr(a->rd, dest, EXT_NONE);
+tcg_gen_st_tl(t1, tcg_env, offsetof(CPULoongArchState, llval));
+gen_set_gpr(a->rd, t1, EXT_NONE);
return true;
}
--
2.25.1
[PULL 0/3] loongarch fixes for 9.0
The following changes since commit c62d54d0a8067ffb3d5b909276f7296d7df33fa7: Update version for v9.0.0-rc0 release (2024-03-19 19:13:52 +) are available in the Git repository at: https://gitlab.com/gaosong/qemu.git tags/pull-loongarch-20240320 for you to fetch changes up to 77642f92c0b71a105aba2a4d03bc62328eae703b: target/loongarch: Fix qemu-loongarch64 hang when executing 'll.d $t0, $t0, 0' (2024-03-20 10:20:08 +0800) pull-loongarch-20240320 Bibo Mao (1): hw/intc/loongarch_extioi: Fix interrupt routing update Song Gao (1): target/loongarch: Fix qemu-loongarch64 hang when executing 'll.d $t0, $t0, 0' Xianglai Li (1): target/loongarch: Fix tlb huge page loading issue hw/intc/loongarch_extioi.c | 2 +- target/loongarch/cpu-csr.h | 3 + target/loongarch/internals.h | 5 - target/loongarch/tcg/insn_trans/trans_atomic.c.inc | 8 +- target/loongarch/tcg/tlb_helper.c | 113 ++--- 5 files changed, 87 insertions(+), 44 deletions(-)
[PULL 1/3] hw/intc/loongarch_extioi: Fix interrupt routing update
From: Bibo Mao
Interrupt number in loop sentence should be base irq plus
loop index, it is missing on checking whether the irq
is pending.
Fixes: 428a6ef4396 ("Add vmstate post_load support")
Signed-off-by: Bibo Mao
Reviewed-by: Song Gao
Signed-off-by: Song Gao
Message-Id: <20240313093932.2653518-1-maob...@loongson.cn>
---
hw/intc/loongarch_extioi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/intc/loongarch_extioi.c b/hw/intc/loongarch_extioi.c
index bdfa3b481e..0b358548eb 100644
--- a/hw/intc/loongarch_extioi.c
+++ b/hw/intc/loongarch_extioi.c
@@ -151,7 +151,7 @@ static inline void extioi_update_sw_coremap(LoongArchExtIOI
*s, int irq,
continue;
}
-if (notify && test_bit(irq, (unsigned long *)s->isr)) {
+if (notify && test_bit(irq + i, (unsigned long *)s->isr)) {
/*
* lower irq at old cpu and raise irq at new cpu
*/
--
2.25.1
[PATCH v2] target/i386: Add new CPU model SierraForest
According to table 1-2 in Intel Architecture Instruction Set Extensions and
Future Features (rev 051) [1], SierraForest has the following new features
which have already been virtualized:
- CMPCCXADD CPUID.(EAX=7,ECX=1):EAX[bit 7]
- AVX-IFMA CPUID.(EAX=7,ECX=1):EAX[bit 23]
- AVX-VNNI-INT8 CPUID.(EAX=7,ECX=1):EDX[bit 4]
- AVX-NE-CONVERT CPUID.(EAX=7,ECX=1):EDX[bit 5]
Add above features to new CPU model SierraForest. Comparing with GraniteRapids
CPU model, SierraForest bare-metal removes the following features:
- HLE CPUID.(EAX=7,ECX=0):EBX[bit 4]
- RTM CPUID.(EAX=7,ECX=0):EBX[bit 11]
- AVX512F CPUID.(EAX=7,ECX=0):EBX[bit 16]
- AVX512DQ CPUID.(EAX=7,ECX=0):EBX[bit 17]
- AVX512_IFMA CPUID.(EAX=7,ECX=0):EBX[bit 21]
- AVX512CD CPUID.(EAX=7,ECX=0):EBX[bit 28]
- AVX512BW CPUID.(EAX=7,ECX=0):EBX[bit 30]
- AVX512VL CPUID.(EAX=7,ECX=0):EBX[bit 31]
- AVX512_VBMI CPUID.(EAX=7,ECX=0):ECX[bit 1]
- AVX512_VBMI2 CPUID.(EAX=7,ECX=0):ECX[bit 6]
- AVX512_VNNI CPUID.(EAX=7,ECX=0):ECX[bit 11]
- AVX512_BITALG CPUID.(EAX=7,ECX=0):ECX[bit 12]
- AVX512_VPOPCNTDQ CPUID.(EAX=7,ECX=0):ECX[bit 14]
- LA57 CPUID.(EAX=7,ECX=0):ECX[bit 16]
- TSXLDTRK CPUID.(EAX=7,ECX=0):EDX[bit 16]
- AMX-BF16 CPUID.(EAX=7,ECX=0):EDX[bit 22]
- AVX512_FP16 CPUID.(EAX=7,ECX=0):EDX[bit 23]
- AMX-TILE CPUID.(EAX=7,ECX=0):EDX[bit 24]
- AMX-INT8 CPUID.(EAX=7,ECX=0):EDX[bit 25]
- AVX512_BF16 CPUID.(EAX=7,ECX=1):EAX[bit 5]
- fast zero-length MOVSB CPUID.(EAX=7,ECX=1):EAX[bit 10]
- fast short CMPSB, SCASB CPUID.(EAX=7,ECX=1):EAX[bit 12]
- AMX-FP16 CPUID.(EAX=7,ECX=1):EAX[bit 21]
- PREFETCHI CPUID.(EAX=7,ECX=1):EDX[bit 14]
- XFD CPUID.(EAX=0xD,ECX=1):EAX[bit 4]
- EPT_PAGE_WALK_LENGTH_5 VMX_EPT_VPID_CAP(0x48c)[bit 7]
Add all features of GraniteRapids CPU model except above features to
SierraForest CPU model.
SierraForest doesn’t support TSX and RTM but supports TAA_NO. When RTM is
not enabled in host, KVM will not report TAA_NO. So, just don't include
TAA_NO in SierraForest CPU model.
[1] https://cdrdv2.intel.com/v1/dl/getContent/671368
Reviewed-by: Zhao Liu
Reviewed-by: Xiaoyao Li
Signed-off-by: Tao Su
---
v1 -> v2:
- Specify the spec rev and table which says the contained features.
- Fix commit message to make it clearer.
- Move the spec link above --- line so that it won’t be gone after commit.
- Add Reviewed-by of Zhao and Xiaoyao.
v1:
- https://lore.kernel.org/all/20231206131923.1192066-1-tao1...@linux.intel.com/
---
target/i386/cpu.c | 126 ++
1 file changed, 126 insertions(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9a210d8d92..8b86698939 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4099,6 +4099,132 @@ static const X86CPUDefinition builtin_x86_defs[] = {
{ /* end of list */ },
},
},
+{
+.name = "SierraForest",
+.level = 0x23,
+.vendor = CPUID_VENDOR_INTEL,
+.family = 6,
+.model = 175,
+.stepping = 0,
+/*
+ * please keep the ascending order so that we can have a clear view of
+ * bit position of each feature.
+ */
+.features[FEAT_1_EDX] =
+CPUID_FP87 | CPUID_VME | CPUID_DE | CPUID_PSE | CPUID_TSC |
+CPUID_MSR | CPUID_PAE | CPUID_MCE | CPUID_CX8 | CPUID_APIC |
+CPUID_SEP | CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV |
+CPUID_PAT | CPUID_PSE36 | CPUID_CLFLUSH | CPUID_MMX | CPUID_FXSR |
+CPUID_SSE | CPUID_SSE2,
+.features[FEAT_1_ECX] =
+CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSSE3 |
+CPUID_EXT_FMA | CPUID_EXT_CX16 | CPUID_EXT_PCID | CPUID_EXT_SSE41 |
+CPUID_EXT_SSE42 | CPUID_EXT_X2APIC | CPUID_EXT_MOVBE |
+CPUID_EXT_POPCNT | CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_AES |
+CPUID_EXT_XSAVE | CPUID_EXT_AVX | CPUID_EXT_F16C |
CPUID_EXT_RDRAND,
+.features[FEAT_8000_0001_EDX] =
+CPUID_EXT2_SYSCALL | CPUID_EXT2_NX | CPUID_EXT2_PDPE1GB |
+CPUID_EXT2_RDTSCP | CPUID_EXT2_LM,
+.features[FEAT_8000_0001_ECX] =
+CPUID_EXT3_LAHF_LM | CPUID_EXT3_ABM | CPUID_EXT3_3DNOWPREFETCH,
+.features[FEAT_8000_0008_EBX] =
+CPUID_8000_0008_EBX_WBNOINVD,
+.features[FEAT_7_0_EBX] =
+CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
+CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS |
+CPUID_7_0_EBX_INVPCID | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX |
+CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT | CPUID_7_0_EBX_CLWB
|
+CPUID_7_0_EBX_SHA_NI,
+.features[FEAT_7_0_ECX] =
+CPUID_7_0_ECX_UMIP | CPUID_7_0_ECX_PKU | CPUID_7_0_ECX_GFNI |
+CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
+CPUID_7_0_ECX_RDPID | CPUID_7_0_ECX_BUS_LOCK_DETECT,
+.features[FEAT_7_0_EDX] =
+CPUID_7_0_EDX_FSRM | CPUID_7_0_EDX_SERIALIZE |
+
[PATCH] contrib/plugins/execlog: Fix compiler warning
1. The g_pattern_match_string() is deprecated when glib2 version >= 2.70.
Use g_pattern_spec_match_string() instead to avoid this problem.
2. The type of second parameter in g_ptr_array_add() is
'gpointer' {aka 'void *'}, but the type of reg->name is 'const char*'.
Cast the type of reg->name to 'gpointer' to avoid this problem.
compiler warning message:
/root/qemu/contrib/plugins/execlog.c:330:17: warning: ‘g_pattern_match_string’
is deprecated: Use 'g_pattern_spec_match_string'
instead [-Wdeprecated-declarations]
330 | if (g_pattern_match_string(pat, rd->name) ||
| ^~
In file included from /usr/include/glib-2.0/glib.h:67,
from /root/qemu/contrib/plugins/execlog.c:9:
/usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
57 | gboolean g_pattern_match_string (GPatternSpec *pspec,
| ^~
/root/qemu/contrib/plugins/execlog.c:331:21: warning: ‘g_pattern_match_string’
is deprecated: Use 'g_pattern_spec_match_string'
instead [-Wdeprecated-declarations]
331 | g_pattern_match_string(pat, rd_lower)) {
| ^~
/usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
57 | gboolean g_pattern_match_string (GPatternSpec *pspec,
| ^~
/root/qemu/contrib/plugins/execlog.c:339:63: warning: passing argument 2 of
‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
339 | g_ptr_array_add(all_reg_names, reg->name);
|~~~^~
In file included from /usr/include/glib-2.0/glib.h:33:
/usr/include/glib-2.0/glib/garray.h:198:62: note: expected
‘gpointer’ {aka ‘void *’} but argument is of type ‘const char *’
198 |gpointer data);
|~~^~~~
Signed-off-by: Yao Xingtao
---
contrib/plugins/execlog.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index a1dfd59ab7..41f6774116 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -327,8 +327,13 @@ static GPtrArray *registers_init(int vcpu_index)
for (int p = 0; p < rmatches->len; p++) {
g_autoptr(GPatternSpec) pat =
g_pattern_spec_new(rmatches->pdata[p]);
g_autofree gchar *rd_lower = g_utf8_strdown(rd->name, -1);
+#if GLIB_VERSION_MAX_ALLOWED < G_ENCODE_VERSION(2, 70)
if (g_pattern_match_string(pat, rd->name) ||
g_pattern_match_string(pat, rd_lower)) {
+#else
+if (g_pattern_spec_match_string(pat, rd->name) ||
+g_pattern_spec_match_string(pat, rd_lower)) {
+#endif
Register *reg = init_vcpu_register(rd);
g_ptr_array_add(registers, reg);
@@ -336,7 +341,7 @@ static GPtrArray *registers_init(int vcpu_index)
if (disas_assist) {
g_mutex_lock(_reg_name_lock);
if (!g_ptr_array_find(all_reg_names, reg->name, NULL))
{
-g_ptr_array_add(all_reg_names, reg->name);
+g_ptr_array_add(all_reg_names,
(gpointer)reg->name);
}
g_mutex_unlock(_reg_name_lock);
}
--
2.37.3
[PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits
The DEXCR emulation was recently changed to a 32-bit register, possibly
because it does have a 32-bit read-only view. It is a full 64-bit
SPR though, so use the corresponding 64-bit write functions.
Fixes: c9de140c2171 ("target/ppc: Fix width of some 32-bit SPRs")
Signed-off-by: Benjamin Gray
---
target/ppc/cpu_init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 7e65f08147..22fdea093b 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -5820,7 +5820,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
{
spr_register(env, SPR_DEXCR, "DEXCR",
SPR_NOACCESS, SPR_NOACCESS,
-_read_generic, _write_generic32,
+_read_generic, _write_generic,
0);
spr_register(env, SPR_UDEXCR, "UDEXCR",
@@ -5831,7 +5831,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
SPR_NOACCESS, SPR_NOACCESS,
SPR_NOACCESS, SPR_NOACCESS,
-_read_generic, _write_generic32,
+_read_generic, _write_generic,
0);
spr_register(env, SPR_UHDEXCR, "UHDEXCR",
--
2.44.0
[PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs
The GDB server protocol assigns an arbitrary numbering of the SPRs.
We track this correspondence on each SPR with gdb_id, using it to
resolve any SPR requests GDB makes.
Early on we generate an XML representation of the SPRs to give GDB,
including this numbering. However the XML is cached globally, and we
skip setting the SPR gdb_id values on subsequent threads if we detect
it is cached. This causes QEMU to fail to resolve SPR requests against
secondary CPUs because it cannot find the matching gdb_id value on that
thread's SPRs.
This is a minimal fix to first assign the gdb_id values, then return
early if the XML is cached. Otherwise we generate the XML using the
now already initialised gdb_id values.
Fixes: 1b53948ff8f7 ("target/ppc: Use GDBFeature for dynamic XML")
Signed-off-by: Benjamin Gray
---
target/ppc/gdbstub.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 3f1e61bdb7..3b28d4e21c 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -305,14 +305,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
unsigned int num_regs = 0;
int i;
-if (pcc->gdb_spr.xml) {
-return;
-}
-
-gdb_feature_builder_init(, >gdb_spr,
- "org.qemu.power.spr", "power-spr.xml",
- cs->gdb_num_regs);
-
for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
ppc_spr_t *spr = >spr_cb[i];
@@ -320,9 +312,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
continue;
}
-gdb_feature_builder_append_reg(, g_ascii_strdown(spr->name,
-1),
- TARGET_LONG_BITS, num_regs,
- "int", "spr");
/*
* GDB identifies registers based on the order they are
* presented in the XML. These ids will not match QEMU's
@@ -335,6 +324,26 @@ static void gdb_gen_spr_feature(CPUState *cs)
num_regs++;
}
+if (pcc->gdb_spr.xml) {
+return;
+}
+
+gdb_feature_builder_init(, >gdb_spr,
+ "org.qemu.power.spr", "power-spr.xml",
+ cs->gdb_num_regs);
+
+for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
+ppc_spr_t *spr = >spr_cb[i];
+
+if (!spr->name) {
+continue;
+}
+
+gdb_feature_builder_append_reg(, g_ascii_strdown(spr->name,
-1),
+ TARGET_LONG_BITS, spr->gdb_id,
+ "int", "spr");
+}
+
gdb_feature_builder_end();
}
#endif
--
2.44.0
Re: [PATCH v2] target/loongarch: Fix qemu-loongarch64 hang when executing 'll.d $t0,$t0, 0'
On 3/19/24 15:39, Song Gao wrote: On gen_ll, if a->imm is zero, make_address_x return src1, but the load to destination may clobber src1. We use a new destination to fix this problem. Fixes: c5af6628f4be (target/loongarch: Extract make_address_i() helper) Suggested-by: Richard Henderson Signed-off-by: Song Gao --- target/loongarch/tcg/insn_trans/trans_atomic.c.inc | 8 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson r~
[PATCH v2] target/loongarch: Fix qemu-loongarch64 hang when executing 'll.d $t0, $t0, 0'
On gen_ll, if a->imm is zero, make_address_x return src1,
but the load to destination may clobber src1. We use a new
destination to fix this problem.
Fixes: c5af6628f4be (target/loongarch: Extract make_address_i() helper)
Suggested-by: Richard Henderson
Signed-off-by: Song Gao
---
target/loongarch/tcg/insn_trans/trans_atomic.c.inc | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
b/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
index 80c2e286fd..974bc2a70f 100644
--- a/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
+++ b/target/loongarch/tcg/insn_trans/trans_atomic.c.inc
@@ -5,14 +5,14 @@
static bool gen_ll(DisasContext *ctx, arg_rr_i *a, MemOp mop)
{
-TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
+TCGv t1 = tcg_temp_new();
TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
TCGv t0 = make_address_i(ctx, src1, a->imm);
-tcg_gen_qemu_ld_i64(dest, t0, ctx->mem_idx, mop);
+tcg_gen_qemu_ld_i64(t1, t0, ctx->mem_idx, mop);
tcg_gen_st_tl(t0, tcg_env, offsetof(CPULoongArchState, lladdr));
-tcg_gen_st_tl(dest, tcg_env, offsetof(CPULoongArchState, llval));
-gen_set_gpr(a->rd, dest, EXT_NONE);
+tcg_gen_st_tl(t1, tcg_env, offsetof(CPULoongArchState, llval));
+gen_set_gpr(a->rd, t1, EXT_NONE);
return true;
}
--
2.25.1
Re: [PATCH] hw/intc/loongarch_extioi: Fix interrupt routing update
在 2024/3/13 下午5:39, Bibo Mao 写道:
Interrupt number in loop sentence should be base irq plus
loop index, it is missing on checking whether the irq
is pending.
Fixes: 428a6ef4396 ("Add vmstate post_load support")
Signed-off-by: Bibo Mao
---
hw/intc/loongarch_extioi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Song Gao
Thanks.
Song Gao
diff --git a/hw/intc/loongarch_extioi.c b/hw/intc/loongarch_extioi.c
index bdfa3b481e..0b358548eb 100644
--- a/hw/intc/loongarch_extioi.c
+++ b/hw/intc/loongarch_extioi.c
@@ -151,7 +151,7 @@ static inline void extioi_update_sw_coremap(LoongArchExtIOI
*s, int irq,
continue;
}
-if (notify && test_bit(irq, (unsigned long *)s->isr)) {
+if (notify && test_bit(irq + i, (unsigned long *)s->isr)) {
/*
* lower irq at old cpu and raise irq at new cpu
*/
[PULL 2/9] target/hppa: Fix assemble_11a insns for wide mode
Tested-by: Helge Deller
Reviewed-by: Helge Deller
Reported-by: Sven Schnelle
Signed-off-by: Richard Henderson
---
target/hppa/insns.decode | 7 ---
target/hppa/translate.c | 23 +--
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
index 0d9f8159ec..9c6f92444c 100644
--- a/target/hppa/insns.decode
+++ b/target/hppa/insns.decode
@@ -24,7 +24,7 @@
%assemble_sr3 13:1 14:2
%assemble_sr3x 13:1 14:2 !function=expand_sr3x
-%assemble_11a 0:s1 4:10!function=expand_shl3
+%assemble_11a 4:12 0:1 !function=expand_11a
%assemble_120:s1 2:1 3:10!function=expand_shl2
%assemble_12a 0:s1 3:11!function=expand_shl2
%assemble_160:16 !function=expand_16
@@ -305,8 +305,9 @@ fstd001011 . . .. . 1 -- 100 0 . .
@fldstdi
# Offset Mem
-@ldstim11 .. b:5 t:5 sp:2 .. \
- disp=%assemble_11a m=%ma2_to_m x=0 scale=0 size=3
+@ldstim11 .. b:5 t:5 \
+ sp=%assemble_sp disp=%assemble_11a \
+m=%ma2_to_m x=0 scale=0 size=3
@ldstim14 .. b:5 t:5 \
sp=%assemble_sp disp=%assemble_16 \
x=0 scale=0 m=0
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index cbe44ef75a..40b9ff6d59 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -121,12 +121,6 @@ static int expand_shl2(DisasContext *ctx, int val)
return val << 2;
}
-/* Used for fp memory ops. */
-static int expand_shl3(DisasContext *ctx, int val)
-{
-return val << 3;
-}
-
/* Used for assemble_21. */
static int expand_shl11(DisasContext *ctx, int val)
{
@@ -144,6 +138,23 @@ static int assemble_6(DisasContext *ctx, int val)
return (val ^ 31) + 1;
}
+/* Expander for assemble_16a(s,cat(im10a,0),i). */
+static int expand_11a(DisasContext *ctx, int val)
+{
+/*
+ * @val is bit 0 and bits [4:15].
+ * Swizzle thing around depending on PSW.W.
+ */
+int im10a = extract32(val, 1, 10);
+int s = extract32(val, 11, 2);
+int i = (-(val & 1) << 13) | (im10a << 3);
+
+if (ctx->tb_flags & PSW_W) {
+i ^= s << 13;
+}
+return i;
+}
+
/* Expander for assemble_16(s,im14). */
static int expand_16(DisasContext *ctx, int val)
{
--
2.34.1
[PULL 1/9] target/hppa: Fix assemble_16 insns for wide mode
Reported-by: Sven Schnelle
Reviewed-by: Helge Deller
Signed-off-by: Richard Henderson
---
target/hppa/insns.decode | 15 +--
target/hppa/translate.c | 22 ++
2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
index f5a3f02fd1..0d9f8159ec 100644
--- a/target/hppa/insns.decode
+++ b/target/hppa/insns.decode
@@ -27,13 +27,14 @@
%assemble_11a 0:s1 4:10!function=expand_shl3
%assemble_120:s1 2:1 3:10!function=expand_shl2
%assemble_12a 0:s1 3:11!function=expand_shl2
+%assemble_160:16 !function=expand_16
%assemble_170:s1 16:5 2:1 3:10 !function=expand_shl2
%assemble_220:s1 16:10 2:1 3:10 !function=expand_shl2
+%assemble_sp14:2 !function=sp0_if_wide
%assemble_210:s1 1:11 14:2 16:5 12:2 !function=expand_shl11
%lowsign_11 0:s1 1:10
-%lowsign_14 0:s1 1:13
%sm_imm 16:10 !function=expand_sm_imm
@@ -221,7 +222,7 @@ sub_b_tsv 10 . . 110100 . .
@rrr_cf_d
ldil001000 t:5 .i=%assemble_21
addil 001010 r:5 .i=%assemble_21
-ldo 001101 b:5 t:5 -- ..i=%lowsign_14
+ldo 001101 b:5 t:5 i=%assemble_16
addi101101 . . 0 ... @rri_cf
addi_tsv101101 . . 1 ... @rri_cf
@@ -306,10 +307,12 @@ fstd001011 . . .. . 1 -- 100 0 .
. @fldstdi
@ldstim11 .. b:5 t:5 sp:2 .. \
disp=%assemble_11a m=%ma2_to_m x=0 scale=0 size=3
-@ldstim14 .. b:5 t:5 sp:2 .. \
- disp=%lowsign_14 x=0 scale=0 m=0
-@ldstim14m .. b:5 t:5 sp:2 .. \
- disp=%lowsign_14 x=0 scale=0 m=%neg_to_m
+@ldstim14 .. b:5 t:5 \
+ sp=%assemble_sp disp=%assemble_16 \
+x=0 scale=0 m=0
+@ldstim14m .. b:5 t:5 \
+ sp=%assemble_sp disp=%assemble_16 \
+x=0 scale=0 m=%neg_to_m
@ldstim12m .. b:5 t:5 sp:2 .. \
disp=%assemble_12a x=0 scale=0 m=%pos_to_m
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index eb2046c5ad..cbe44ef75a 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -144,6 +144,28 @@ static int assemble_6(DisasContext *ctx, int val)
return (val ^ 31) + 1;
}
+/* Expander for assemble_16(s,im14). */
+static int expand_16(DisasContext *ctx, int val)
+{
+/*
+ * @val is bits [0:15], containing both im14 and s.
+ * Swizzle thing around depending on PSW.W.
+ */
+int s = extract32(val, 14, 2);
+int i = (-(val & 1) << 13) | extract32(val, 1, 13);
+
+if (ctx->tb_flags & PSW_W) {
+i ^= s << 13;
+}
+return i;
+}
+
+/* The sp field is only present with !PSW_W. */
+static int sp0_if_wide(DisasContext *ctx, int sp)
+{
+return ctx->tb_flags & PSW_W ? 0 : sp;
+}
+
/* Translate CMPI doubleword conditions to standard. */
static int cmpbid_c(DisasContext *ctx, int val)
{
--
2.34.1
[PULL 0/9] target/hppa fixes for 9.0
The following changes since commit c62d54d0a8067ffb3d5b909276f7296d7df33fa7: Update version for v9.0.0-rc0 release (2024-03-19 19:13:52 +) are available in the Git repository at: https://gitlab.com/rth7680/qemu.git tags/pull-pa-20240319 for you to fetch changes up to 518d2f4300e5c50a3e6416fd46e58373781a5267: target/hppa: fix do_stdby_e() (2024-03-19 14:08:02 -1000) target/hppa: Fix load/store offset assembly for wide mode target/hppa: Fix LDCW,S shift target/hppa: Fix SHRPD conditions target/hppa: Fix access_id checks target/hppa: Exit TB after Flush Instruction Cache target/hppa: Fix MFIA result target hppa: Fix STDBY,E Richard Henderson (3): target/hppa: Fix assemble_16 insns for wide mode target/hppa: Fix assemble_11a insns for wide mode target/hppa: Fix assemble_12a insns for wide mode Sven Schnelle (6): target/hppa: ldcw,s uses static shift of 3 target/hppa: fix shrp for wide mode target/hppa: fix access_id check target/hppa: exit tb on flush cache instructions target/hppa: mask privilege bits in mfia target/hppa: fix do_stdby_e() target/hppa/insns.decode | 55 +++-- target/hppa/mem_helper.c | 80 +--- target/hppa/op_helper.c | 10 +++--- target/hppa/translate.c | 77 -- 4 files changed, 166 insertions(+), 56 deletions(-)
[PULL 9/9] target/hppa: fix do_stdby_e()
From: Sven Schnelle
stdby,e,m was writing data from the wrong half of the register
into memory for cases 0-3.
Fixes: 25460fc5a71 ("target/hppa: Implement STDBY")
Signed-off-by: Sven Schnelle
Reviewed-by: Richard Henderson
Message-Id: <20240319161921.487080-7-sv...@stackframe.org>
Signed-off-by: Richard Henderson
---
target/hppa/op_helper.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
index 480fe80844..6cf49f33b7 100644
--- a/target/hppa/op_helper.c
+++ b/target/hppa/op_helper.c
@@ -281,17 +281,17 @@ static void do_stdby_e(CPUHPPAState *env, target_ulong
addr, uint64_t val,
case 3:
/* The 3 byte store must appear atomic. */
if (parallel) {
-atomic_store_mask32(env, addr - 3, val, 0xff00u, ra);
+atomic_store_mask32(env, addr - 3, val >> 32, 0xff00u, ra);
} else {
-cpu_stw_data_ra(env, addr - 3, val >> 16, ra);
-cpu_stb_data_ra(env, addr - 1, val >> 8, ra);
+cpu_stw_data_ra(env, addr - 3, val >> 48, ra);
+cpu_stb_data_ra(env, addr - 1, val >> 40, ra);
}
break;
case 2:
-cpu_stw_data_ra(env, addr - 2, val >> 16, ra);
+cpu_stw_data_ra(env, addr - 2, val >> 48, ra);
break;
case 1:
-cpu_stb_data_ra(env, addr - 1, val >> 24, ra);
+cpu_stb_data_ra(env, addr - 1, val >> 56, ra);
break;
default:
/* Nothing is stored, but protection is checked and the
--
2.34.1
[PULL 7/9] target/hppa: exit tb on flush cache instructions
From: Sven Schnelle
When the guest modifies the tb it is currently executing from,
it executes a fic instruction. Exit the tb on such instruction,
otherwise we might execute stale code.
Signed-off-by: Sven Schnelle
Message-Id: <20240319161921.487080-5-sv...@stackframe.org>
Signed-off-by: Richard Henderson
---
target/hppa/insns.decode | 6 +++---
target/hppa/translate.c | 7 +++
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
index 5412ff9836..f58455dfdb 100644
--- a/target/hppa/insns.decode
+++ b/target/hppa/insns.decode
@@ -144,9 +144,9 @@ getshadowregs 1101 1110 1010 1101 0010
nop 01 - - -- 11001010 0 - # fdc, disp
nop_addrx 01 . . -- 01001010 . - @addrx # fdc, index
nop_addrx 01 . . -- 01001011 . - @addrx # fdce
-nop_addrx 01 . . --- 0001010 . - @addrx # fic 0x0a
-nop_addrx 01 . . -- 0100 . 0 @addrx # fic 0x4f
-nop_addrx 01 . . --- 0001011 . - @addrx # fice
+fic 01 . . --- 0001010 . - @addrx # fic 0x0a
+fic 01 . . -- 0100 . 0 @addrx # fic 0x4f
+fic 01 . . --- 0001011 . - @addrx # fice
nop_addrx 01 . . -- 01001110 . 0 @addrx # pdc
probe 01 b:5 ri:5 sp:2 imm:1 100011 write:1 0 t:5
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 29ef061baf..107d7f1a85 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -2343,6 +2343,13 @@ static bool trans_nop_addrx(DisasContext *ctx, arg_ldst
*a)
return true;
}
+static bool trans_fic(DisasContext *ctx, arg_ldst *a)
+{
+/* End TB for flush instruction cache, so we pick up new insns. */
+ctx->base.is_jmp = DISAS_IAQ_N_STALE;
+return trans_nop_addrx(ctx, a);
+}
+
static bool trans_probe(DisasContext *ctx, arg_probe *a)
{
TCGv_i64 dest, ofs;
--
2.34.1
[PULL 4/9] target/hppa: ldcw,s uses static shift of 3
From: Sven Schnelle
Fixes: 96d6407f363 ("target-hppa: Implement loads and stores")
Signed-off-by: Sven Schnelle
Reviewed-by: Richard Henderson
Message-Id: <20240319161921.487080-2-sv...@stackframe.org>
Signed-off-by: Richard Henderson
---
target/hppa/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index be0b0494d0..47c6db78c7 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -3135,7 +3135,7 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
dest = dest_gpr(ctx, a->t);
}
-form_gva(ctx, , , a->b, a->x, a->scale ? a->size : 0,
+form_gva(ctx, , , a->b, a->x, a->scale ? 3 : 0,
a->disp, a->sp, a->m, MMU_DISABLED(ctx));
/*
--
2.34.1
[PULL 6/9] target/hppa: fix access_id check
From: Sven Schnelle
PA2.0 provides 8 instead of 4 PID registers.
Signed-off-by: Sven Schnelle
Reviewed-by: Richard Henderson
Message-Id: <20240319161921.487080-4-sv...@stackframe.org>
Signed-off-by: Richard Henderson
---
target/hppa/mem_helper.c | 80 +++-
1 file changed, 62 insertions(+), 18 deletions(-)
diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
index 80f51e753f..84785b5a5c 100644
--- a/target/hppa/mem_helper.c
+++ b/target/hppa/mem_helper.c
@@ -152,6 +152,49 @@ static HPPATLBEntry *hppa_alloc_tlb_ent(CPUHPPAState *env)
return ent;
}
+#define ACCESS_ID_MASK 0x
+
+/* Return the set of protections allowed by a PID match. */
+static int match_prot_id_1(uint32_t access_id, uint32_t prot_id)
+{
+if (((access_id ^ (prot_id >> 1)) & ACCESS_ID_MASK) == 0) {
+return (prot_id & 1
+? PAGE_EXEC | PAGE_READ
+: PAGE_EXEC | PAGE_READ | PAGE_WRITE);
+}
+return 0;
+}
+
+static int match_prot_id32(CPUHPPAState *env, uint32_t access_id)
+{
+int r, i;
+
+for (i = CR_PID1; i <= CR_PID4; ++i) {
+r = match_prot_id_1(access_id, env->cr[i]);
+if (r) {
+return r;
+}
+}
+return 0;
+}
+
+static int match_prot_id64(CPUHPPAState *env, uint32_t access_id)
+{
+int r, i;
+
+for (i = CR_PID1; i <= CR_PID4; ++i) {
+r = match_prot_id_1(access_id, env->cr[i]);
+if (r) {
+return r;
+}
+r = match_prot_id_1(access_id, env->cr[i] >> 32);
+if (r) {
+return r;
+}
+}
+return 0;
+}
+
int hppa_get_physical_address(CPUHPPAState *env, vaddr addr, int mmu_idx,
int type, hwaddr *pphys, int *pprot,
HPPATLBEntry **tlb_entry)
@@ -224,29 +267,30 @@ int hppa_get_physical_address(CPUHPPAState *env, vaddr
addr, int mmu_idx,
break;
}
-/* access_id == 0 means public page and no check is performed */
-if (ent->access_id && MMU_IDX_TO_P(mmu_idx)) {
-/* If bits [31:1] match, and bit 0 is set, suppress write. */
-int match = ent->access_id * 2 + 1;
-
-if (match == env->cr[CR_PID1] || match == env->cr[CR_PID2] ||
-match == env->cr[CR_PID3] || match == env->cr[CR_PID4]) {
-prot &= PAGE_READ | PAGE_EXEC;
-if (type == PAGE_WRITE) {
-ret = EXCP_DMPI;
-goto egress;
-}
-}
-}
-
-/* No guest access type indicates a non-architectural access from
- within QEMU. Bypass checks for access, D, B and T bits. */
+/*
+ * No guest access type indicates a non-architectural access from
+ * within QEMU. Bypass checks for access, D, B, P and T bits.
+ */
if (type == 0) {
goto egress;
}
+/* access_id == 0 means public page and no check is performed */
+if (ent->access_id && MMU_IDX_TO_P(mmu_idx)) {
+int access_prot = (hppa_is_pa20(env)
+ ? match_prot_id64(env, ent->access_id)
+ : match_prot_id32(env, ent->access_id));
+if (unlikely(!(type & access_prot))) {
+/* Not allowed -- Inst/Data Memory Protection Id Fault. */
+ret = type & PAGE_EXEC ? EXCP_IMP : EXCP_DMPI;
+goto egress;
+}
+/* Otherwise exclude permissions not allowed (i.e WD). */
+prot &= access_prot;
+}
+
if (unlikely(!(prot & type))) {
-/* The access isn't allowed -- Inst/Data Memory Protection Fault. */
+/* Not allowed -- Inst/Data Memory Access Rights Fault. */
ret = (type & PAGE_EXEC) ? EXCP_IMP : EXCP_DMAR;
goto egress;
}
--
2.34.1
[PULL 8/9] target/hppa: mask privilege bits in mfia
From: Sven Schnelle
mfia should return only the iaoq bits without privilege
bits.
Fixes: 98a9cb792c8 ("target-hppa: Implement system and memory-management insns")
Signed-off-by: Sven Schnelle
Reviewed-by: Richard Henderson
Reviewed-by: Helge Deller
Message-Id: <20240319161921.487080-6-sv...@stackframe.org>
Signed-off-by: Richard Henderson
---
target/hppa/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 107d7f1a85..19594f917e 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -2011,7 +2011,7 @@ static bool trans_mfia(DisasContext *ctx, arg_mfia *a)
{
unsigned rt = a->t;
TCGv_i64 tmp = dest_gpr(ctx, rt);
-tcg_gen_movi_i64(tmp, ctx->iaoq_f);
+tcg_gen_movi_i64(tmp, ctx->iaoq_f & ~3ULL);
save_gpr(ctx, rt, tmp);
cond_free(>null_cond);
--
2.34.1
[PULL 3/9] target/hppa: Fix assemble_12a insns for wide mode
Tested-by: Helge Deller
Reported-by: Sven Schnelle
Signed-off-by: Richard Henderson
---
target/hppa/insns.decode | 27 ---
target/hppa/translate.c | 17 +
2 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/target/hppa/insns.decode b/target/hppa/insns.decode
index 9c6f92444c..5412ff9836 100644
--- a/target/hppa/insns.decode
+++ b/target/hppa/insns.decode
@@ -26,7 +26,7 @@
%assemble_11a 4:12 0:1 !function=expand_11a
%assemble_120:s1 2:1 3:10!function=expand_shl2
-%assemble_12a 0:s1 3:11!function=expand_shl2
+%assemble_12a 3:13 0:1 !function=expand_12a
%assemble_160:16 !function=expand_16
%assemble_170:s1 16:5 2:1 3:10 !function=expand_shl2
%assemble_220:s1 16:10 2:1 3:10 !function=expand_shl2
@@ -314,8 +314,9 @@ fstd001011 . . .. . 1 -- 100 0 . .
@fldstdi
@ldstim14m .. b:5 t:5 \
sp=%assemble_sp disp=%assemble_16 \
x=0 scale=0 m=%neg_to_m
-@ldstim12m .. b:5 t:5 sp:2 .. \
- disp=%assemble_12a x=0 scale=0 m=%pos_to_m
+@ldstim12m .. b:5 t:5 \
+ sp=%assemble_sp disp=%assemble_12a \
+x=0 scale=0 m=%pos_to_m
# LDB, LDH, LDW, LDWM
ld 01 . . .. ..@ldstim14 size=0
@@ -331,15 +332,19 @@ st 011010 . . .. ..
@ldstim14 size=2
st 011011 . . .. ..@ldstim14m size=2
st 01 . . .. ...10.@ldstim12m size=2
-fldw010110 b:5 . sp:2 ..\
- disp=%assemble_12a t=%rm64 m=%a_to_m x=0 scale=0 size=2
-fldw010111 b:5 . sp:2 ...0..\
- disp=%assemble_12a t=%rm64 m=0 x=0 scale=0 size=2
+fldw010110 b:5 . \
+ disp=%assemble_12a sp=%assemble_sp \
+t=%rm64 m=%a_to_m x=0 scale=0 size=2
+fldw010111 b:5 . .0..\
+ disp=%assemble_12a sp=%assemble_sp \
+t=%rm64 m=0 x=0 scale=0 size=2
-fstw00 b:5 . sp:2 ..\
- disp=%assemble_12a t=%rm64 m=%a_to_m x=0 scale=0 size=2
-fstw01 b:5 . sp:2 ...0..\
- disp=%assemble_12a t=%rm64 m=0 x=0 scale=0 size=2
+fstw00 b:5 . \
+ disp=%assemble_12a sp=%assemble_sp \
+t=%rm64 m=%a_to_m x=0 scale=0 size=2
+fstw01 b:5 . .0..\
+ disp=%assemble_12a sp=%assemble_sp \
+t=%rm64 m=0 x=0 scale=0 size=2
ld 010100 . . .. 0.@ldstim11
fldd010100 . . .. 1.@ldstim11
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 40b9ff6d59..be0b0494d0 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -155,6 +155,23 @@ static int expand_11a(DisasContext *ctx, int val)
return i;
}
+/* Expander for assemble_16a(s,im11a,i). */
+static int expand_12a(DisasContext *ctx, int val)
+{
+/*
+ * @val is bit 0 and bits [3:15].
+ * Swizzle thing around depending on PSW.W.
+ */
+int im11a = extract32(val, 1, 11);
+int s = extract32(val, 12, 2);
+int i = (-(val & 1) << 13) | (im11a << 2);
+
+if (ctx->tb_flags & PSW_W) {
+i ^= s << 13;
+}
+return i;
+}
+
/* Expander for assemble_16(s,im14). */
static int expand_16(DisasContext *ctx, int val)
{
--
2.34.1
[PULL 5/9] target/hppa: fix shrp for wide mode
From: Sven Schnelle
Fixes: f7b775a9c075 ("target/hppa: Implement SHRPD")
Signed-off-by: Sven Schnelle
Reviewed-by: Richard Henderson
Reviewed-by: Helge Deller
Message-Id: <20240319161921.487080-3-sv...@stackframe.org>
Signed-off-by: Richard Henderson
---
target/hppa/translate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 47c6db78c7..29ef061baf 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -3512,7 +3512,7 @@ static bool trans_shrp_sar(DisasContext *ctx,
arg_shrp_sar *a)
/* Install the new nullification. */
cond_free(>null_cond);
if (a->c) {
-ctx->null_cond = do_sed_cond(ctx, a->c, false, dest);
+ctx->null_cond = do_sed_cond(ctx, a->c, a->d, dest);
}
return nullify_end(ctx);
}
@@ -3555,7 +3555,7 @@ static bool trans_shrp_imm(DisasContext *ctx,
arg_shrp_imm *a)
/* Install the new nullification. */
cond_free(>null_cond);
if (a->c) {
-ctx->null_cond = do_sed_cond(ctx, a->c, false, dest);
+ctx->null_cond = do_sed_cond(ctx, a->c, a->d, dest);
}
return nullify_end(ctx);
}
--
2.34.1
Re: [PATCH] target/i386: Export RFDS bit to guests
On Wed, Mar 20, 2024 at 08:23:39AM +0800, Xiaoyao Li wrote: > On 3/19/2024 11:08 PM, Pawan Gupta wrote: > > On Tue, Mar 19, 2024 at 12:22:08PM +0800, Xiaoyao Li wrote: > > > On 3/13/2024 10:53 PM, Pawan Gupta wrote: > > > > Register File Data Sampling (RFDS) is a CPU side-channel vulnerability > > > > that may expose stale register value. CPUs that set RFDS_NO bit in MSR > > > > IA32_ARCH_CAPABILITIES indicate that they are not vulnerable to RFDS. > > > > Similarly, RFDS_CLEAR indicates that CPU is affected by RFDS, and has > > > > the microcode to help mitigate RFDS. > > > > > > > > Make RFDS_CLEAR and RFDS_NO bits available to guests. > > > > > > What's the status of KVM part? > > > > KVM part is already upstreamed and backported: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v6.8.1=50d33b98b1e23d1cd8743b3cac7a0ae5718b8b00 > > I see. It was not sent to kvm maillist and not merged through KVM tree. > > With KVM part in palce, > > Reviewed-by: Xiaoyao Li Thanks.
Re: [PATCH] target/i386: Export RFDS bit to guests
On 3/19/2024 11:08 PM, Pawan Gupta wrote: On Tue, Mar 19, 2024 at 12:22:08PM +0800, Xiaoyao Li wrote: On 3/13/2024 10:53 PM, Pawan Gupta wrote: Register File Data Sampling (RFDS) is a CPU side-channel vulnerability that may expose stale register value. CPUs that set RFDS_NO bit in MSR IA32_ARCH_CAPABILITIES indicate that they are not vulnerable to RFDS. Similarly, RFDS_CLEAR indicates that CPU is affected by RFDS, and has the microcode to help mitigate RFDS. Make RFDS_CLEAR and RFDS_NO bits available to guests. What's the status of KVM part? KVM part is already upstreamed and backported: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v6.8.1=50d33b98b1e23d1cd8743b3cac7a0ae5718b8b00 I see. It was not sent to kvm maillist and not merged through KVM tree. With KVM part in palce, Reviewed-by: Xiaoyao Li
Re: [PATCH 2/4] target/riscv: Add right functions to set agnostic elements
On 3/19/24 11:57, Daniel Henrique Barboza wrote:
This seems correct but a bit over complicated at first glance. I wonder if we
have
something simpler already done somewhere.
Richard, does ARM (or any other arch) do anything of the sort? Aside from more
trivial
byte swaps using bswap64() I didn't find anything similar.
No, nothing quite like.
We recently posted a big endian related fix here:
[PATCH for 9.0 v15 03/10] target/riscv/vector_helper.c: fix 'vmvr_v' memcpy
endianess
But not sure how to apply it here.
It's almost exactly the same, only with memset instead of memcpy.
if (HOST_BIG_ENDIAN && idx % 8 != 0) {
uint32_t j = ROUND_UP(idx, 8);
memset(vd + H(j - 1), -1, j - idx);
idx = j;
}
memset(vd + idx, -1, tot - idx);
I'll note that you don't need to change the api of vext_set_elems_1s -- so most of these
patches are not required.
r~
Re: [PATCH] Revert mapped-ram multifd support to fd: URI
On Tue, Mar 19, 2024 at 06:09:41PM -0300, Fabiano Rosas wrote: > This reverts commit decdc76772c453ff1444612e910caa0d45cd8eac in full > and also the relevant migration-tests from > 7a09f092834641b7a793d50a3a261073bbb404a6. > > After the addition of the new QAPI-based migration address API in 8.2 > we've been converting an "fd:" URI into a SocketAddress, missing the > fact that the "fd:" syntax could also be used for a plain file instead > of a socket. This is a problem because the SocketAddress is part of > the API, so we're effectively asking users to create a "socket" > channel to pass in a plain file. > > The easiest way to fix this situation is to deprecate the usage of > both SocketAddress and "fd:" when used with a plain file for > migration. Since this has been possible since 8.2, we can wait until > 9.1 to deprecate it. > > For 9.0, however, we should avoid adding further support to migration > to a plain file using the old "fd:" syntax or the new SocketAddress > API, and instead require the usage of either the old-style "file:" URI > or the FileMigrationArgs::filename field of the new API with the > "/dev/fdset/NN" syntax, both of which are already supported. > > Signed-off-by: Fabiano Rosas queued for 9.0-rc1. -- Peter Xu
Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
On Tue, Mar 19, 2024 at 08:48:39PM +, Het Gala wrote:
> 'object' can return NULL if there is no socket-address, such as with a
> file migration. Then the visitor code below fails and the test crashes.
>
> Ignore and return NULL when socket-address is missing in the reply so
> we don't break future tests that use a non-socket type.
Hmm, this patch isn't as clear to me. Even if this can return NULL now,
it'll soon crash at some later point, no?
IMHO such patch is more suitable to be included in the same patch where
such new tests will be introduced, then we're addressing some real test
code changes that will work, rather than worrying on what will happen in
the future (and as I mentioned, i don't think it fully resolved that either..)
Thanks,
>
> Suggested-by: Fabiano Rosas
> Signed-off-by: Het Gala
> ---
> tests/qtest/migration-helpers.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index b2a90469fb..fb7156f09a 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -90,6 +90,10 @@ static SocketAddress
> *migrate_get_socket_address(QTestState *who)
> QObject *object;
>
> rsp = migrate_query(who);
> +
> +if (!qdict_haskey(rsp, "socket-address")) {
> +return NULL;
> +}
> object = qdict_get(rsp, "socket-address");
>
> iv = qobject_input_visitor_new(object);
> --
> 2.22.3
>
--
Peter Xu
Re: [PATCH 2/4] target/riscv: Add right functions to set agnostic elements
(--- CCing Richard ---)
On 3/6/24 06:20, Huang Tao wrote:
We add vext_set_elems_1s to set agnostic elements to 1s in both big
and little endian situation.
In the function vext_set_elems_1s. We using esz argument to get the first
element to set. 'cnt' is just idx * esz.
Signed-off-by: Huang Tao
---
target/riscv/vector_internals.c | 53 +
target/riscv/vector_internals.h | 2 ++
2 files changed, 55 insertions(+)
diff --git a/target/riscv/vector_internals.c b/target/riscv/vector_internals.c
index 349b24f4ae..455be96996 100644
--- a/target/riscv/vector_internals.c
+++ b/target/riscv/vector_internals.c
@@ -20,6 +20,59 @@
#include "vector_internals.h"
/* set agnostic elements to 1s */
+#if HOST_BIG_ENDIAN
+void vext_set_elems_1s(void *vd, uint32_t is_agnostic, uint32_t esz,
+ uint32_t idx, uint32_t tot)
+{
+if (is_agnostic == 0) {
+/* policy undisturbed */
+return;
+}
+void *base = NULL;
+switch (esz) {
+case 1:
+base = ((int8_t *)vd + H1(idx));
+break;
+case 2:
+base = ((int16_t *)vd + H2(idx));
+break;
+case 4:
+base = ((int32_t *)vd + H4(idx));
+break;
+case 8:
+base = ((int64_t *)vd + H8(idx));
+break;
+default:
+g_assert_not_reached();
+break;
+}
+/*
+ * spilt the elements into 2 parts
+ * part_begin: the memory need to be set in the first uint64_t unit
+ * part_allign: the memory need to be set begins from next uint64_t
+ * unit and alligned to 8
+ */
+uint32_t cnt = idx * esz;
+int part_begin, part_allign;
+part_begin = MIN(tot - cnt, 8 - (cnt % 8));
+part_allign = ((tot - cnt - part_begin) / 8) * 8;
+
+memset(base - part_begin + 1, -1, part_begin);
+memset(QEMU_ALIGN_PTR_UP(base, 8), -1, part_allign);
This seems correct but a bit over complicated at first glance. I wonder if we
have
something simpler already done somewhere.
Richard, does ARM (or any other arch) do anything of the sort? Aside from more
trivial
byte swaps using bswap64() I didn't find anything similar.
We recently posted a big endian related fix here:
[PATCH for 9.0 v15 03/10] target/riscv/vector_helper.c: fix 'vmvr_v' memcpy
endianess
But not sure how to apply it here.
Thanks,
Daniel
+}
+#else
+void vext_set_elems_1s(void *vd, uint32_t is_agnostic, uint32_t esz,
+ uint32_t idx, uint32_t tot)
+{
+if (is_agnostic == 0) {
+/* policy undisturbed */
+return;
+}
+uint32_t cnt = idx * esz;
+memset(vd + cnt, -1, tot - cnt);
+}
+#endif
+
void vext_set_elems_1s_le(void *base, uint32_t is_agnostic, uint32_t cnt,
uint32_t tot)
{
diff --git a/target/riscv/vector_internals.h b/target/riscv/vector_internals.h
index fa599f60ca..c96e52f926 100644
--- a/target/riscv/vector_internals.h
+++ b/target/riscv/vector_internals.h
@@ -114,6 +114,8 @@ static inline uint32_t vext_get_total_elems(CPURISCVState
*env, uint32_t desc,
}
/* set agnostic elements to 1s */
+void vext_set_elems_1s(void *vd, uint32_t is_agnostic, uint32_t esz,
+ uint32_t idx, uint32_t tot);
void vext_set_elems_1s_le(void *base, uint32_t is_agnostic, uint32_t cnt,
uint32_t tot);
Re: [PATCH 2/2] tests/qtest/migration: Fix typo for vsock in SocketAddress_to_str
On Tue, Mar 19, 2024 at 08:48:40PM +, Het Gala wrote:
> Signed-off-by: Het Gala
> ---
> tests/qtest/migration-helpers.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index fb7156f09a..651c6c555a 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -42,7 +42,7 @@ static char *SocketAddress_to_str(SocketAddress *addr)
> case SOCKET_ADDRESS_TYPE_FD:
> return g_strdup_printf("fd:%s", addr->u.fd.str);
> case SOCKET_ADDRESS_TYPE_VSOCK:
> -return g_strdup_printf("tcp:%s:%s",
> +return g_strdup_printf("vsock:%s:%s",
> addr->u.vsock.cid,
> addr->u.vsock.port);
> default:
Queued for 9.1, thanks.
--
Peter Xu
Re: [PATCH 1/4] target/riscv: Rename vext_set_elems_1s function
On 3/6/24 06:20, Huang Tao wrote: In RVV and vcrypto instructions, the masked and tail elements are set to 1s using vext_set_elems_1s function if the vma/vta bit is set. It is the element agnostic policy. However, this function can't deal the big endian situation. We rename the function, adding '_le' to the end of the name, to indicate that it only suits little endian situation. Signed-off-by: Huang Tao --- Given that this is a temporary change (vext_set_elems_1s_le is removed later) it's ok to not bother with the macro identations. Reviewed-by: Daniel Henrique Barboza target/riscv/vcrypto_helper.c | 32 ++-- target/riscv/vector_helper.c| 92 - target/riscv/vector_internals.c | 10 ++-- target/riscv/vector_internals.h | 6 +-- 4 files changed, 70 insertions(+), 70 deletions(-) diff --git a/target/riscv/vcrypto_helper.c b/target/riscv/vcrypto_helper.c index e2d719b13b..f818749a63 100644 --- a/target/riscv/vcrypto_helper.c +++ b/target/riscv/vcrypto_helper.c @@ -235,7 +235,7 @@ static inline void xor_round_key(AESState *round_state, AESState *round_key) } \ env->vstart = 0; \ /* set tail elements to 1s */ \ -vext_set_elems_1s(vd, vta, vl * 4, total_elems * 4); \ +vext_set_elems_1s_le(vd, vta, vl * 4, total_elems * 4); \ } #define GEN_ZVKNED_HELPER_VS(NAME, ...) \ @@ -259,7 +259,7 @@ static inline void xor_round_key(AESState *round_state, AESState *round_key) } \ env->vstart = 0; \ /* set tail elements to 1s */ \ -vext_set_elems_1s(vd, vta, vl * 4, total_elems * 4); \ +vext_set_elems_1s_le(vd, vta, vl * 4, total_elems * 4); \ } GEN_ZVKNED_HELPER_VV(vaesef_vv, aesenc_SB_SR_AK(_state, @@ -339,7 +339,7 @@ void HELPER(vaeskf1_vi)(void *vd_vptr, void *vs2_vptr, uint32_t uimm, } env->vstart = 0; /* set tail elements to 1s */ -vext_set_elems_1s(vd, vta, vl * 4, total_elems * 4); +vext_set_elems_1s_le(vd, vta, vl * 4, total_elems * 4); } void HELPER(vaeskf2_vi)(void *vd_vptr, void *vs2_vptr, uint32_t uimm, @@ -396,7 +396,7 @@ void HELPER(vaeskf2_vi)(void *vd_vptr, void *vs2_vptr, uint32_t uimm, } env->vstart = 0; /* set tail elements to 1s */ -vext_set_elems_1s(vd, vta, vl * 4, total_elems * 4); +vext_set_elems_1s_le(vd, vta, vl * 4, total_elems * 4); } static inline uint32_t sig0_sha256(uint32_t x) @@ -469,7 +469,7 @@ void HELPER(vsha2ms_vv)(void *vd, void *vs1, void *vs2, CPURISCVState *env, } /* set tail elements to 1s */ total_elems = vext_get_total_elems(env, desc, esz); -vext_set_elems_1s(vd, vta, env->vl * esz, total_elems * esz); +vext_set_elems_1s_le(vd, vta, env->vl * esz, total_elems * esz); env->vstart = 0; } @@ -579,7 +579,7 @@ void HELPER(vsha2ch32_vv)(void *vd, void *vs1, void *vs2, CPURISCVState *env, /* set tail elements to 1s */ total_elems = vext_get_total_elems(env, desc, esz); -vext_set_elems_1s(vd, vta, env->vl * esz, total_elems * esz); +vext_set_elems_1s_le(vd, vta, env->vl * esz, total_elems * esz); env->vstart = 0; } @@ -597,7 +597,7 @@ void HELPER(vsha2ch64_vv)(void *vd, void *vs1, void *vs2, CPURISCVState *env, /* set tail elements to 1s */ total_elems = vext_get_total_elems(env, desc, esz); -vext_set_elems_1s(vd, vta, env->vl * esz, total_elems * esz); +vext_set_elems_1s_le(vd, vta, env->vl * esz, total_elems * esz); env->vstart = 0; } @@ -615,7 +615,7 @@ void HELPER(vsha2cl32_vv)(void *vd, void *vs1, void *vs2, CPURISCVState *env, /* set tail elements to 1s */ total_elems = vext_get_total_elems(env, desc, esz); -vext_set_elems_1s(vd, vta, env->vl * esz, total_elems * esz); +vext_set_elems_1s_le(vd, vta, env->vl * esz, total_elems * esz); env->vstart = 0; } @@ -633,7 +633,7 @@ void HELPER(vsha2cl64_vv)(void *vd, void *vs1, void *vs2, CPURISCVState *env, /* set tail elements to 1s */ total_elems = vext_get_total_elems(env, desc, esz); -vext_set_elems_1s(vd, vta, env->vl * esz, total_elems * esz); +vext_set_elems_1s_le(vd, vta, env->vl * esz, total_elems * esz); env->vstart = 0; } @@ -672,7 +672,7 @@ void HELPER(vsm3me_vv)(void *vd_vptr, void *vs1_vptr, void *vs2_vptr, vd[(i * 8) + j] = bswap32(w[H4(j + 16)]); } } -vext_set_elems_1s(vd_vptr, vta, env->vl * esz, total_elems * esz); +vext_set_elems_1s_le(vd_vptr, vta, env->vl * esz, total_elems *
Re: [PATCH 05/22] plugins: Move function pointer in qemu_plugin_dyn_cb
On 3/19/24 03:18, Pierrick Bouvier wrote:
On 3/16/24 05:57, Richard Henderson wrote:
The out-of-line function pointer is mutually exclusive
with inline expansion, so move it into the union.
Wrap the pointer in a structure named 'regular' to match
PLUGIN_CB_REGULAR.
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 4 +++-
accel/tcg/plugin-gen.c | 4 ++--
plugins/core.c | 8
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 12a96cea2a..143262dca8 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -84,13 +84,15 @@ enum plugin_dyn_cb_subtype {
* instance of a callback to be called upon the execution of a particular TB.
*/
struct qemu_plugin_dyn_cb {
- union qemu_plugin_cb_sig f;
void *userp;
enum plugin_dyn_cb_subtype type;
/* @rw applies to mem callbacks only (both regular and inline) */
enum qemu_plugin_mem_rw rw;
/* fields specific to each dyn_cb type go here */
union {
+ struct {
+ union qemu_plugin_cb_sig f;
+ } regular;
struct {
qemu_plugin_u64 entry;
enum qemu_plugin_op op;
While we are cleaning this, maybe this could be only a union (moving rw and userp to
fields), and only type + union would be used.
Even if we duplicate userp in regular, and mem_cb, it would be much more
readable.
For instance, userp is never used by inline operations.
I was wondering about this. But I was also thinking about your follow-on patch set to add
conditional calls: do we want a multiplicity of union members, or will we want separate
bits and pieces that can be strung together?
E.g.
TCGCond cond; /* ALWAYS, or compare entry vs imm. */
/* PLUGIN_CB_REGULAR_COND or PLUGIN_CB_INLINE_* */
qemu_plugin_u64 entry;
uint64_t imm;
/* PLUGIN_CB_REGULAR_* */
union qemu_plugin_cb_sig f;
void *userp;
r~
Re: [PATCH-for-9.1 19/27] target/riscv: Convert to TCGCPUOps::get_cpu_state()
On 3/19/24 12:42, Philippe Mathieu-Daudé wrote:
Convert cpu_get_tb_cpu_state() to TCGCPUOps::get_cpu_state().
Note, now riscv_get_cpu_state() is restricted to TCG, and
is declared with static scope.
Signed-off-by: Philippe Mathieu-Daudé
---
Reviewed-by: Daniel Henrique Barboza
target/riscv/cpu.h | 3 -
target/riscv/cpu.c | 2 +-
target/riscv/cpu_helper.c | 87
target/riscv/tcg/tcg-cpu.c | 88 +
target/riscv/insn_trans/trans_rvv.c.inc | 2 +-
5 files changed, 90 insertions(+), 92 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 3b1a02b944..d00d1be235 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -704,9 +704,6 @@ static inline uint32_t vext_get_vlmax(uint32_t vlenb,
uint32_t vsew,
return vlen >> (vsew + 3 - lmul);
}
-void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
- uint64_t *cs_base, uint32_t *pflags);
-
void riscv_cpu_update_mask(CPURISCVState *env);
bool riscv_cpu_is_32bit(RISCVCPU *cpu);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c160b9216b..ca537d0e0a 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -889,7 +889,7 @@ static vaddr riscv_cpu_get_pc(CPUState *cs)
RISCVCPU *cpu = RISCV_CPU(cs);
CPURISCVState *env = >env;
-/* Match cpu_get_tb_cpu_state. */
+/* Match riscv_get_cpu_state. */
if (env->xl == MXL_RV32) {
return env->pc & UINT32_MAX;
}
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index ce7322011d..e18a269358 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -25,7 +25,6 @@
#include "pmu.h"
#include "exec/exec-all.h"
#include "instmap.h"
-#include "tcg/tcg-op.h"
#include "trace.h"
#include "semihosting/common-semi.h"
#include "sysemu/cpu-timers.h"
@@ -62,92 +61,6 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
#endif
}
-void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
- uint64_t *cs_base, uint32_t *pflags)
-{
-RISCVCPU *cpu = env_archcpu(env);
-RISCVExtStatus fs, vs;
-uint32_t flags = 0;
-
-*pc = env->xl == MXL_RV32 ? env->pc & UINT32_MAX : env->pc;
-*cs_base = 0;
-
-if (cpu->cfg.ext_zve32f) {
-/*
- * If env->vl equals to VLMAX, we can use generic vector operation
- * expanders (GVEC) to accerlate the vector operations.
- * However, as LMUL could be a fractional number. The maximum
- * vector size can be operated might be less than 8 bytes,
- * which is not supported by GVEC. So we set vl_eq_vlmax flag to true
- * only when maxsz >= 8 bytes.
- */
-
-/* lmul encoded as in DisasContext::lmul */
-int8_t lmul = sextract32(FIELD_EX64(env->vtype, VTYPE, VLMUL), 0, 3);
-uint32_t vsew = FIELD_EX64(env->vtype, VTYPE, VSEW);
-uint32_t vlmax = vext_get_vlmax(cpu->cfg.vlenb, vsew, lmul);
-uint32_t maxsz = vlmax << vsew;
-bool vl_eq_vlmax = (env->vstart == 0) && (vlmax == env->vl) &&
- (maxsz >= 8);
-flags = FIELD_DP32(flags, TB_FLAGS, VILL, env->vill);
-flags = FIELD_DP32(flags, TB_FLAGS, SEW, vsew);
-flags = FIELD_DP32(flags, TB_FLAGS, LMUL,
- FIELD_EX64(env->vtype, VTYPE, VLMUL));
-flags = FIELD_DP32(flags, TB_FLAGS, VL_EQ_VLMAX, vl_eq_vlmax);
-flags = FIELD_DP32(flags, TB_FLAGS, VTA,
- FIELD_EX64(env->vtype, VTYPE, VTA));
-flags = FIELD_DP32(flags, TB_FLAGS, VMA,
- FIELD_EX64(env->vtype, VTYPE, VMA));
-flags = FIELD_DP32(flags, TB_FLAGS, VSTART_EQ_ZERO, env->vstart == 0);
-} else {
-flags = FIELD_DP32(flags, TB_FLAGS, VILL, 1);
-}
-
-#ifdef CONFIG_USER_ONLY
-fs = EXT_STATUS_DIRTY;
-vs = EXT_STATUS_DIRTY;
-#else
-flags = FIELD_DP32(flags, TB_FLAGS, PRIV, env->priv);
-
-flags |= riscv_env_mmu_index(env, 0);
-fs = get_field(env->mstatus, MSTATUS_FS);
-vs = get_field(env->mstatus, MSTATUS_VS);
-
-if (env->virt_enabled) {
-flags = FIELD_DP32(flags, TB_FLAGS, VIRT_ENABLED, 1);
-/*
- * Merge DISABLED and !DIRTY states using MIN.
- * We will set both fields when dirtying.
- */
-fs = MIN(fs, get_field(env->mstatus_hs, MSTATUS_FS));
-vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS));
-}
-
-/* With Zfinx, floating point is enabled/disabled by Smstateen. */
-if (!riscv_has_ext(env, RVF)) {
-fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == RISCV_EXCP_NONE)
- ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED;
-}
-
-if (cpu->cfg.debug && !icount_enabled()) {
-flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
-}
-#endif
-
-flags = FIELD_DP32(flags,
qemu fuzz crash in virtio_net_queue_reset()
Hi all! From fuzzing I've got a fuzz-data, which produces the following crash: qemu-fuzz-x86_64: ../hw/net/virtio-net.c:134: void flush_or_purge_queued_packets(NetClientState *): Assertion `!virtio_net_get_subqueue(nc)->async_tx.elem' failed. ==2172308== ERROR: libFuzzer: deadly signal #0 0x5bd8c748b5a1 in __sanitizer_print_stack_trace (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26f05a1) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) #1 0x5bd8c73fde38 in fuzzer::PrintStackTrace() (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2662e38) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) #2 0x5bd8c73e38b3 in fuzzer::Fuzzer::CrashCallback() (/home/settlements/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26488b3) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) #3 0x739eec84251f (/lib/x86_64-linux-gnu/libc.so.6+0x4251f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e) #4 0x739eec8969fb in __pthread_kill_implementation nptl/./nptl/pthread_kill.c:43:17 #5 0x739eec8969fb in __pthread_kill_internal nptl/./nptl/pthread_kill.c:78:10 #6 0x739eec8969fb in pthread_kill nptl/./nptl/pthread_kill.c:89:10 #7 0x739eec842475 in gsignal signal/../sysdeps/posix/raise.c:26:13 #8 0x739eec8287f2 in abort stdlib/./stdlib/abort.c:79:7 #9 0x739eec82871a in __assert_fail_base assert/./assert/assert.c:92:3 #10 0x739eec839e95 in __assert_fail assert/./assert/assert.c:101:3 #11 0x5bd8c995d9e2 in flush_or_purge_queued_packets /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:134:5 #12 0x5bd8c9918a5f in virtio_net_queue_reset /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:563:5 #13 0x5bd8c9b724e5 in virtio_queue_reset /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio.c:2492:9 #14 0x5bd8c8bcfb7c in virtio_pci_common_write /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio-pci.c:1372:13 #15 0x5bd8c9e19cf3 in memory_region_write_accessor /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:492:5 #16 0x5bd8c9e19631 in access_with_adjusted_size /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:554:18 #17 0x5bd8c9e17f3c in memory_region_dispatch_write /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:1514:16 #18 0x5bd8c9ea3bbe in flatview_write_continue /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2825:23 #19 0x5bd8c9e91aab in flatview_write /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2867:12 #20 0x5bd8c9e91568 in address_space_write /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2963:18 #21 0x5bd8c74c8a90 in __wrap_qtest_writeq /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/qtest_wrappers.c:187:9 #22 0x5bd8c74dc4da in op_write /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:487:13 #23 0x5bd8c74d942e in generic_fuzz /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:714:17 #24 0x5bd8c74c016e in LLVMFuzzerTestOneInput /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/fuzz.c:152:5 #25 0x5bd8c73e4e43 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2649e43) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) #26 0x5bd8c73cebbf in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2633bbf) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) #27 0x5bd8c73d4916 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2639916) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) #28 0x5bd8c73fe732 in main (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2663732) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) #29 0x739eec829d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #30 0x739eec829e3f in __libc_start_main csu/../csu/libc-start.c:392:3 #31 0x5bd8c73c9484 in _start (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x262e484) (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) How to reproduce: ./configure --target-list=x86_64-softmmu --enable-debug --disable-docs --cc=clang --cxx=clang++ --enable-fuzzing --enable-sanitizers --enable-slirp make -j20 qemu-fuzz-x86_64 ./build/qemu-fuzz-x86_64 --fuzz-target=generic-fuzz-virtio-net-pci-slirp ../generic-fuzz-virtio-net-pci-slirp.crash-7707e14adea64d129be88faeb6ca57dab6118ec5 This ...crash-7707... file is attached. git-bisect points to 7dc6be52f4ead25e7da8fb758900bdcb527996f7 "virtio-net: support queue reset" as a first bad commit. That's a commit which introduces
Re: [PATCH 01/22] tcg: Add TCGContext.emit_before_op
On 3/19/24 04:04, Alex Bennée wrote:
Richard Henderson writes:
Allow operations to be emitted via normal expanders
into the middle of the opcode stream.
Signed-off-by: Richard Henderson
---
include/tcg/tcg.h | 1 +
tcg/tcg.c | 14 --
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 451f3fec41..e9d05f40b0 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -552,6 +552,7 @@ struct TCGContext {
QTAILQ_HEAD(, TCGOp) ops, free_ops;
QSIMPLEQ_HEAD(, TCGLabel) labels;
+TCGOp *emit_before_op;
Could we add some kdoc comments to the TCGContext describing what each
variables is for. Is this just a list of ops to emit before the current
instruction emulation? Is it cleared between instruction boundaries?
It's not a list, it's an insertion point.
Nothing automatic about it; manually controlled by plugin-gen.c.
(I believe it will be useful for cleanup in tcg/optimize.c as well.)
r~
Re: [PATCH 1/5] target/riscv: Add support for Zve32x extension
Hi Jason,
Care to re-send please? The patches don't apply to neither riscv-to-apply.next
nor master.
Thanks,
Daniel
On 3/19/24 13:23, Jason Chien wrote:
Ping. Can anyone review the patches please?
Jason Chien mailto:jason.ch...@sifive.com>> 於
2024年3月7日 週四 上午1:09寫道：
Add support for Zve32x extension and replace some checks for Zve32f with
Zve32x, since Zve32f depends on Zve32x.
Signed-off-by: Jason Chien mailto:jason.ch...@sifive.com>>
Reviewed-by: Frank Chang mailto:frank.ch...@sifive.com>>
Reviewed-by: Max Chou mailto:max.c...@sifive.com>>
---
target/riscv/cpu.c | 1 +
target/riscv/cpu_cfg.h | 1 +
target/riscv/cpu_helper.c | 2 +-
target/riscv/csr.c | 2 +-
target/riscv/insn_trans/trans_rvv.c.inc | 4 ++--
target/riscv/tcg/tcg-cpu.c | 16
6 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index fd0c7efdda..10ccae3323 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -152,6 +152,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
ISA_EXT_DATA_ENTRY(zvbb, PRIV_VERSION_1_12_0, ext_zvbb),
ISA_EXT_DATA_ENTRY(zvbc, PRIV_VERSION_1_12_0, ext_zvbc),
ISA_EXT_DATA_ENTRY(zve32f, PRIV_VERSION_1_10_0, ext_zve32f),
+ ISA_EXT_DATA_ENTRY(zve32x, PRIV_VERSION_1_10_0, ext_zve32x),
ISA_EXT_DATA_ENTRY(zve64f, PRIV_VERSION_1_10_0, ext_zve64f),
ISA_EXT_DATA_ENTRY(zve64d, PRIV_VERSION_1_10_0, ext_zve64d),
ISA_EXT_DATA_ENTRY(zvfbfmin, PRIV_VERSION_1_12_0, ext_zvfbfmin),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index be39870691..beb3d10213 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -90,6 +90,7 @@ struct RISCVCPUConfig {
bool ext_zhinx;
bool ext_zhinxmin;
bool ext_zve32f;
+ bool ext_zve32x;
bool ext_zve64f;
bool ext_zve64d;
bool ext_zvbb;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index c994a72634..ebbe56d9a2 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -72,7 +72,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
*pc = env->xl == MXL_RV32 ? env->pc & UINT32_MAX : env->pc;
*cs_base = 0;
- if (cpu->cfg.ext_zve32f) {
+ if (cpu->cfg.ext_zve32x) {
/*
* If env->vl equals to VLMAX, we can use generic vector operation
* expanders (GVEC) to accerlate the vector operations.
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 726096444f..d96feea5d3 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -93,7 +93,7 @@ static RISCVException fs(CPURISCVState *env, int csrno)
static RISCVException vs(CPURISCVState *env, int csrno)
{
- if (riscv_cpu_cfg(env)->ext_zve32f) {
+ if (riscv_cpu_cfg(env)->ext_zve32x) {
#if !defined(CONFIG_USER_ONLY)
if (!env->debugger && !riscv_cpu_vector_enabled(env)) {
return RISCV_EXCP_ILLEGAL_INST;
diff --git a/target/riscv/insn_trans/trans_rvv.c.inc
b/target/riscv/insn_trans/trans_rvv.c.inc
index 9e101ab434..f00f1ee886 100644
--- a/target/riscv/insn_trans/trans_rvv.c.inc
+++ b/target/riscv/insn_trans/trans_rvv.c.inc
@@ -149,7 +149,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1,
TCGv s2)
{
TCGv s1, dst;
- if (!require_rvv(s) || !s->cfg_ptr->ext_zve32f) {
+ if (!require_rvv(s) || !s->cfg_ptr->ext_zve32x) {
return false;
}
@@ -179,7 +179,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv
s1, TCGv s2)
{
TCGv dst;
- if (!require_rvv(s) || !s->cfg_ptr->ext_zve32f) {
+ if (!require_rvv(s) || !s->cfg_ptr->ext_zve32x) {
return false;
}
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index ab6db817db..ce539528e6 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -501,9 +501,13 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu,
Error **errp)
return;
}
- if (cpu->cfg.ext_zve32f && !riscv_has_ext(env, RVF)) {
- error_setg(errp, "Zve32f/Zve64f extensions require F extension");
- return;
+ /* The Zve32f extension depends on the Zve32x extension */
+ if (cpu->cfg.ext_zve32f) {
+ if (!riscv_has_ext(env, RVF)) {
+ error_setg(errp, "Zve32f/Zve64f extensions require F
extension");
+ return;
+ }
+ cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve32x), true);
}
if (cpu->cfg.ext_zvfh) {
@@ -653,13 +657,9 @@ void
Re: [PATCH-for-9.1 00/27] accel/tcg: Introduce TCGCPUOps::get_cpu_state() handler
On 3/19/24 05:42, Philippe Mathieu-Daudé wrote: Philippe Mathieu-Daudé (27): accel/tcg: Ensure frontends define restore_state_to_opc handler accel/tcg: Introduce TCGCPUOps::get_cpu_state() handler target/alpha: Convert to TCGCPUOps::get_cpu_state() target/arm: Restrict TCG-specific declarations target/arm: Convert to TCGCPUOps::get_cpu_state() target/avr: Convert to TCGCPUOps::get_cpu_state() target/cris: Convert to TCGCPUOps::get_cpu_state() target/hexagon: Convert to TCGCPUOps::get_cpu_state() target/hppa: Convert to TCGCPUOps::get_cpu_state() target/i386: Convert to TCGCPUOps::get_cpu_state() target/loongarch: Convert to TCGCPUOps::get_cpu_state() target/m68k: Convert to TCGCPUOps::get_cpu_state() target/microblaze: Convert to TCGCPUOps::get_cpu_state() target/mips: Convert to TCGCPUOps::get_cpu_state() target/nios2: Convert to TCGCPUOps::get_cpu_state() target/openrisc: Convert to TCGCPUOps::get_cpu_state() target/ppc: Indent ppc_tcg_ops[] with 4 spaces target/ppc: Convert to TCGCPUOps::get_cpu_state() target/riscv: Convert to TCGCPUOps::get_cpu_state() target/rx: Convert to TCGCPUOps::get_cpu_state() target/s390x: Restrict TCG-specific declarations target/s390x: Convert to TCGCPUOps::get_cpu_state() target/sh4: Convert to TCGCPUOps::get_cpu_state() target/sparc: Convert to TCGCPUOps::get_cpu_state() target/tricore: Convert to TCGCPUOps::get_cpu_state() target/xtensa: Convert to TCGCPUOps::get_cpu_state() accel/tcg: Remove check on TARGET_HAS_CPU_GET_TB_CPU_STATE Modulo 3 notes, Reviewed-by: Richard Henderson r~
Re: [PATCH-for-9.1 24/27] target/sparc: Convert to TCGCPUOps::get_cpu_state()
On 3/19/24 05:42, Philippe Mathieu-Daudé wrote: Convert cpu_get_tb_cpu_state() to TCGCPUOps::get_cpu_state(). Signed-off-by: Philippe Mathieu-Daudé --- target/sparc/cpu.h | 37 ++--- target/sparc/cpu.c | 1 + target/sparc/translate.c | 33 + 3 files changed, 36 insertions(+), 35 deletions(-) Again, why translate.c? r~
[PATCH] Revert mapped-ram multifd support to fd: URI
This reverts commit decdc76772c453ff1444612e910caa0d45cd8eac in full
and also the relevant migration-tests from
7a09f092834641b7a793d50a3a261073bbb404a6.
After the addition of the new QAPI-based migration address API in 8.2
we've been converting an "fd:" URI into a SocketAddress, missing the
fact that the "fd:" syntax could also be used for a plain file instead
of a socket. This is a problem because the SocketAddress is part of
the API, so we're effectively asking users to create a "socket"
channel to pass in a plain file.
The easiest way to fix this situation is to deprecate the usage of
both SocketAddress and "fd:" when used with a plain file for
migration. Since this has been possible since 8.2, we can wait until
9.1 to deprecate it.
For 9.0, however, we should avoid adding further support to migration
to a plain file using the old "fd:" syntax or the new SocketAddress
API, and instead require the usage of either the old-style "file:" URI
or the FileMigrationArgs::filename field of the new API with the
"/dev/fdset/NN" syntax, both of which are already supported.
Signed-off-by: Fabiano Rosas
---
CI run: https://gitlab.com/farosas/qemu/-/pipelines/1219876558
---
migration/fd.c | 56
migration/fd.h | 2 --
migration/file.c | 19 ++--
migration/migration.c| 13 -
migration/multifd.c | 2 --
tests/qtest/migration-test.c | 43 ---
6 files changed, 8 insertions(+), 127 deletions(-)
diff --git a/migration/fd.c b/migration/fd.c
index fe0d096abd..449adaa2de 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -15,42 +15,19 @@
*/
#include "qemu/osdep.h"
-#include "qapi/error.h"
#include "channel.h"
#include "fd.h"
#include "file.h"
#include "migration.h"
#include "monitor/monitor.h"
-#include "io/channel-file.h"
-#include "io/channel-socket.h"
#include "io/channel-util.h"
-#include "options.h"
#include "trace.h"
-static struct FdOutgoingArgs {
-int fd;
-} outgoing_args;
-
-int fd_args_get_fd(void)
-{
-return outgoing_args.fd;
-}
-
-void fd_cleanup_outgoing_migration(void)
-{
-if (outgoing_args.fd > 0) {
-close(outgoing_args.fd);
-outgoing_args.fd = -1;
-}
-}
-
void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error
**errp)
{
QIOChannel *ioc;
int fd = monitor_get_fd(monitor_cur(), fdname, errp);
-int newfd;
-
if (fd == -1) {
return;
}
@@ -62,18 +39,6 @@ void fd_start_outgoing_migration(MigrationState *s, const
char *fdname, Error **
return;
}
-/*
- * This is dup()ed just to avoid referencing an fd that might
- * be already closed by the iochannel.
- */
-newfd = dup(fd);
-if (newfd == -1) {
-error_setg_errno(errp, errno, "Could not dup FD %d", fd);
-object_unref(ioc);
-return;
-}
-outgoing_args.fd = newfd;
-
qio_channel_set_name(ioc, "migration-fd-outgoing");
migration_channel_connect(s, ioc, NULL, NULL);
object_unref(OBJECT(ioc));
@@ -104,20 +69,9 @@ void fd_start_incoming_migration(const char *fdname, Error
**errp)
return;
}
-if (migrate_multifd()) {
-if (fd_is_socket(fd)) {
-error_setg(errp,
- "Multifd migration to a socket FD is not supported");
-object_unref(ioc);
-return;
-}
-
-file_create_incoming_channels(ioc, errp);
-} else {
-qio_channel_set_name(ioc, "migration-fd-incoming");
-qio_channel_add_watch_full(ioc, G_IO_IN,
- fd_accept_incoming_migration,
- NULL, NULL,
- g_main_context_get_thread_default());
-}
+qio_channel_set_name(ioc, "migration-fd-incoming");
+qio_channel_add_watch_full(ioc, G_IO_IN,
+ fd_accept_incoming_migration,
+ NULL, NULL,
+ g_main_context_get_thread_default());
}
diff --git a/migration/fd.h b/migration/fd.h
index 0c0a18d9e7..b901bc014e 100644
--- a/migration/fd.h
+++ b/migration/fd.h
@@ -20,6 +20,4 @@ void fd_start_incoming_migration(const char *fdname, Error
**errp);
void fd_start_outgoing_migration(MigrationState *s, const char *fdname,
Error **errp);
-void fd_cleanup_outgoing_migration(void);
-int fd_args_get_fd(void);
#endif
diff --git a/migration/file.c b/migration/file.c
index b6e8ba13f2..ab18ba505a 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -11,7 +11,6 @@
#include "qemu/error-report.h"
#include "qapi/error.h"
#include "channel.h"
-#include "fd.h"
#include "file.h"
#include "migration.h"
#include "io/channel-file.h"
@@ -55,27 +54,15 @@ bool file_send_channel_create(gpointer opaque, Error **errp)
{
QIOChannelFile *ioc;
int flags = O_WRONLY;
-bool ret =
Re: [RFC PATCH-for-9.1 8/8] target/microblaze: Widen $ear to 64-bit
On Tue, Mar 19, 2024 at 07:28:55AM +0100, Philippe Mathieu-Daudé wrote:
> The Exception Address Register is 64-bit wide.
> User emulation only access the 32 lower bits.
Reviewed-by: Edgar E. Iglesias
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/microblaze/cpu.h | 2 +-
> linux-user/elfload.c | 2 +-
> target/microblaze/gdbstub.c | 2 +-
> target/microblaze/translate.c | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
> index c3e2aba0ec..a9f93b37b7 100644
> --- a/target/microblaze/cpu.h
> +++ b/target/microblaze/cpu.h
> @@ -251,7 +251,7 @@ struct CPUArchState {
> uint32_t pc;
> uint32_t msr;/* All bits of MSR except MSR[C] and MSR[CC] */
> uint32_t msr_c; /* MSR[C], in low bit; other bits must be 0 */
> -target_ulong ear;
> +uint64_t ear;
> uint32_t esr;
> uint32_t fsr;
> uint32_t btr;
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 60cf55b36c..4612aef95a 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1498,7 +1498,7 @@ static void elf_core_copy_regs(target_elf_gregset_t
> *regs, const CPUMBState *env
> (*regs)[pos++] = tswapreg(env->pc);
> (*regs)[pos++] = tswapreg(mb_cpu_read_msr(env));
> (*regs)[pos++] = 0;
> -(*regs)[pos++] = tswapreg(env->ear);
> +(*regs)[pos++] = tswapreg((uint32_t)env->ear);
> (*regs)[pos++] = 0;
> (*regs)[pos++] = tswapreg(env->esr);
> }
> diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
> index 09d74e164d..147d20c3e4 100644
> --- a/target/microblaze/gdbstub.c
> +++ b/target/microblaze/gdbstub.c
> @@ -63,7 +63,7 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray
> *mem_buf, int n)
> val = mb_cpu_read_msr(env);
> break;
> case GDB_EAR:
> -val = env->ear;
> +val = (uint32_t)env->ear;
> break;
> case GDB_ESR:
> val = env->esr;
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index 493850c544..19b180501f 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1835,7 +1835,7 @@ void mb_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> }
>
> qemu_fprintf(f, "\nesr=0x%04x fsr=0x%02x btr=0x%08x edr=0x%x\n"
> - "ear=0x" TARGET_FMT_lx " slr=0x%x shr=0x%x\n",
> + "ear=0x%" PRIx64 " slr=0x%x shr=0x%x\n",
> env->esr, env->fsr, env->btr, env->edr,
> env->ear, env->slr, env->shr);
>
> --
> 2.41.0
>
Re: [PATCH-for-9.1 22/27] target/s390x: Convert to TCGCPUOps::get_cpu_state()
On 3/19/24 05:42, Philippe Mathieu-Daudé wrote: Convert cpu_get_tb_cpu_state() to TCGCPUOps::get_cpu_state(). Note, now s390x_get_cpu_state() is restricted to TCG. Signed-off-by: Philippe Mathieu-Daudé --- target/s390x/cpu.h| 30 -- target/s390x/s390x-internal.h | 2 ++ target/s390x/cpu.c| 1 + target/s390x/tcg/mem_helper.c | 2 +- target/s390x/tcg/translate.c | 23 +++ 5 files changed, 27 insertions(+), 31 deletions(-) Why is the function in translate.c, not cpu.c (with or without ifdefs)? r~
Re: [PATCH-for-9.1 10/27] target/i386: Convert to TCGCPUOps::get_cpu_state()
On 3/19/24 05:42, Philippe Mathieu-Daudé wrote: +static inline void x86_get_cpu_state(CPUX86State *env, vaddr *pc, Remove inline. r~
Re: [PATCH-for-9.1 7/8] target/microblaze: Move MMU helpers to sys_helper.c
On Tue, Mar 19, 2024 at 07:28:54AM +0100, Philippe Mathieu-Daudé wrote:
> MMU helpers are only used during system emulation,
> move them to sys_helper.c.
Reviewed-by: Edgar E. Iglesias
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/microblaze/op_helper.c | 48 --
> target/microblaze/sys_helper.c | 47 +
> 2 files changed, 47 insertions(+), 48 deletions(-)
>
> diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
> index f6378030b7..45dbed4aaa 100644
> --- a/target/microblaze/op_helper.c
> +++ b/target/microblaze/op_helper.c
> @@ -381,51 +381,3 @@ void helper_stackprot(CPUMBState *env, target_ulong addr)
> cpu_loop_exit_restore(cs, GETPC());
> }
> }
> -
> -#if !defined(CONFIG_USER_ONLY)
> -/* Writes/reads to the MMU's special regs end up here. */
> -uint32_t helper_mmu_read(CPUMBState *env, uint32_t ext, uint32_t rn)
> -{
> -return mmu_read(env, ext, rn);
> -}
> -
> -void helper_mmu_write(CPUMBState *env, uint32_t ext, uint32_t rn, uint32_t v)
> -{
> -mmu_write(env, ext, rn, v);
> -}
> -
> -void mb_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
> - unsigned size, MMUAccessType access_type,
> - int mmu_idx, MemTxAttrs attrs,
> - MemTxResult response, uintptr_t retaddr)
> -{
> -MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
> -CPUMBState *env = >env;
> -
> -qemu_log_mask(CPU_LOG_INT, "Transaction failed: vaddr 0x%" VADDR_PRIx
> - " physaddr 0x" HWADDR_FMT_plx " size %d access type %s\n",
> - addr, physaddr, size,
> - access_type == MMU_INST_FETCH ? "INST_FETCH" :
> - (access_type == MMU_DATA_LOAD ? "DATA_LOAD" :
> "DATA_STORE"));
> -
> -if (!(env->msr & MSR_EE)) {
> -return;
> -}
> -
> -if (access_type == MMU_INST_FETCH) {
> -if (!cpu->cfg.iopb_bus_exception) {
> -return;
> -}
> -env->esr = ESR_EC_INSN_BUS;
> -} else {
> -if (!cpu->cfg.dopb_bus_exception) {
> -return;
> -}
> -env->esr = ESR_EC_DATA_BUS;
> -}
> -
> -env->ear = addr;
> -cs->exception_index = EXCP_HW_EXCP;
> -cpu_loop_exit_restore(cs, retaddr);
> -}
> -#endif
> diff --git a/target/microblaze/sys_helper.c b/target/microblaze/sys_helper.c
> index 5180500354..7531f95ca7 100644
> --- a/target/microblaze/sys_helper.c
> +++ b/target/microblaze/sys_helper.c
> @@ -21,6 +21,7 @@
> #include "qemu/osdep.h"
> #include "cpu.h"
> #include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
> #include "qemu/host-utils.h"
> #include "exec/log.h"
>
> @@ -292,3 +293,49 @@ void mb_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> cs->exception_index = EXCP_HW_EXCP;
> cpu_loop_exit(cs);
> }
> +
> +/* Writes/reads to the MMU's special regs end up here. */
> +uint32_t helper_mmu_read(CPUMBState *env, uint32_t ext, uint32_t rn)
> +{
> +return mmu_read(env, ext, rn);
> +}
> +
> +void helper_mmu_write(CPUMBState *env, uint32_t ext, uint32_t rn, uint32_t v)
> +{
> +mmu_write(env, ext, rn, v);
> +}
> +
> +void mb_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
> + unsigned size, MMUAccessType access_type,
> + int mmu_idx, MemTxAttrs attrs,
> + MemTxResult response, uintptr_t retaddr)
> +{
> +MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
> +CPUMBState *env = >env;
> +
> +qemu_log_mask(CPU_LOG_INT, "Transaction failed: vaddr 0x%" VADDR_PRIx
> + " physaddr 0x" HWADDR_FMT_plx " size %d access type %s\n",
> + addr, physaddr, size,
> + access_type == MMU_INST_FETCH ? "INST_FETCH" :
> + (access_type == MMU_DATA_LOAD ? "DATA_LOAD" :
> "DATA_STORE"));
> +
> +if (!(env->msr & MSR_EE)) {
> +return;
> +}
> +
> +if (access_type == MMU_INST_FETCH) {
> +if (!cpu->cfg.iopb_bus_exception) {
> +return;
> +}
> +env->esr = ESR_EC_INSN_BUS;
> +} else {
> +if (!cpu->cfg.dopb_bus_exception) {
> +return;
> +}
> +env->esr = ESR_EC_DATA_BUS;
> +}
> +
> +env->ear = addr;
> +cs->exception_index = EXCP_HW_EXCP;
> +cpu_loop_exit_restore(cs, retaddr);
> +}
> --
> 2.41.0
>
Re: [PATCH-for-9.1 6/8] target/microblaze: Rename helper.c -> sys_helper.c
On Tue, Mar 19, 2024 at 07:28:53AM +0100, Philippe Mathieu-Daudé wrote:
> helper.c only contains system emulation helpers,
> rename it as sys_helper.c.
> Adapt meson and remove pointless #ifdef'ry.
Reviewed-by: Edgar E. Iglesias
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/microblaze/{helper.c => sys_helper.c} | 5 +
> target/microblaze/meson.build| 2 +-
> 2 files changed, 2 insertions(+), 5 deletions(-)
> rename target/microblaze/{helper.c => sys_helper.c} (99%)
>
> diff --git a/target/microblaze/helper.c b/target/microblaze/sys_helper.c
> similarity index 99%
> rename from target/microblaze/helper.c
> rename to target/microblaze/sys_helper.c
> index 3f410fc7b5..5180500354 100644
> --- a/target/microblaze/helper.c
> +++ b/target/microblaze/sys_helper.c
> @@ -1,5 +1,5 @@
> /*
> - * MicroBlaze helper routines.
> + * MicroBlaze system helper routines.
> *
> * Copyright (c) 2009 Edgar E. Iglesias
> * Copyright (c) 2009-2012 PetaLogix Qld Pty Ltd.
> @@ -24,7 +24,6 @@
> #include "qemu/host-utils.h"
> #include "exec/log.h"
>
> -#ifndef CONFIG_USER_ONLY
> static bool mb_cpu_access_is_secure(MicroBlazeCPU *cpu,
> MMUAccessType access_type)
> {
> @@ -266,8 +265,6 @@ bool mb_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
> return false;
> }
>
> -#endif /* !CONFIG_USER_ONLY */
> -
> void mb_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> MMUAccessType access_type,
> int mmu_idx, uintptr_t retaddr)
> diff --git a/target/microblaze/meson.build b/target/microblaze/meson.build
> index 3ed4fbb67a..013ea542be 100644
> --- a/target/microblaze/meson.build
> +++ b/target/microblaze/meson.build
> @@ -5,7 +5,6 @@ microblaze_ss.add(gen)
> microblaze_ss.add(files(
>'cpu.c',
>'gdbstub.c',
> - 'helper.c',
>'op_helper.c',
>'translate.c',
> ))
> @@ -14,6 +13,7 @@ microblaze_system_ss = ss.source_set()
> microblaze_system_ss.add(files(
>'mmu.c',
>'machine.c',
> + 'sys_helper.c',
> ))
>
> target_arch += {'microblaze': microblaze_ss}
> --
> 2.41.0
>
Re: [PATCH-for-9.1 4/8] target/microblaze: Use 32-bit destination in gen_goto_tb()
On Tue, Mar 19, 2024 at 07:28:51AM +0100, Philippe Mathieu-Daudé wrote:
> cpu_pc and jmp_dest are 32-bit.
>
Reviewed-by: Edgar E. Iglesias
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/microblaze/translate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index 4e52ef32db..d6a42381bb 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -121,7 +121,7 @@ static void gen_raise_hw_excp(DisasContext *dc, uint32_t
> esr_ec)
> gen_raise_exception_sync(dc, EXCP_HW_EXCP);
> }
>
> -static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
> +static void gen_goto_tb(DisasContext *dc, int n, uint32_t dest)
> {
> if (translator_use_goto_tb(>base, dest)) {
> tcg_gen_goto_tb(n);
> --
> 2.41.0
>
Re: [PATCH-for-9.1 3/8] target/microblaze: Widen vaddr in mmu_translate()
On Tue, Mar 19, 2024 at 07:28:50AM +0100, Philippe Mathieu-Daudé wrote:
> Use 'vaddr' type for virtual addresses.
Reviewed-by: Edgar E. Iglesias
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/microblaze/mmu.h | 2 +-
> target/microblaze/mmu.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/microblaze/mmu.h b/target/microblaze/mmu.h
> index 1068bd2d52..2aca39c923 100644
> --- a/target/microblaze/mmu.h
> +++ b/target/microblaze/mmu.h
> @@ -86,7 +86,7 @@ typedef struct {
> } MicroBlazeMMULookup;
>
> unsigned int mmu_translate(MicroBlazeCPU *cpu, MicroBlazeMMULookup *lu,
> - target_ulong vaddr, MMUAccessType rw, int
> mmu_idx);
> + vaddr vaddr, MMUAccessType rw, int mmu_idx);
> uint32_t mmu_read(CPUMBState *env, bool ea, uint32_t rn);
> void mmu_write(CPUMBState *env, bool ea, uint32_t rn, uint32_t v);
> void mmu_init(MicroBlazeMMU *mmu);
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index 234006634e..eb7c683020 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -75,7 +75,7 @@ static void mmu_change_pid(CPUMBState *env, unsigned int
> newpid)
>
> /* rw - 0 = read, 1 = write, 2 = fetch. */
> unsigned int mmu_translate(MicroBlazeCPU *cpu, MicroBlazeMMULookup *lu,
> - target_ulong vaddr, MMUAccessType rw, int mmu_idx)
> + vaddr vaddr, MMUAccessType rw, int mmu_idx)
> {
> MicroBlazeMMU *mmu = >env.mmu;
> unsigned int i, hit = 0;
> --
> 2.41.0
>
Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address
On 20/03/24 12:33 am, Fabiano Rosas wrote:
Het Gala writes:
On 18/03/24 7:46 pm, Fabiano Rosas wrote:
Het Gala writes:
On 15/03/24 6:28 pm, Fabiano Rosas wrote:
Het Galawrites:
Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
+object = qdict_get(rsp, "socket-address");
Just a heads up, none of what I'm about to say applies to current
master.
This can return NULL if there is no socket-address, such as with a file
migration. Then the visitor code below just barfs. It would be nice if
we touched this up eventually.
Yes. I agree this is not full proof solution and covers for all the cases.
It would only for 'socket-address'. Could you elaborate on what other than
socket-address the QObject can have ?
I can just not have the socket-address, AFAICS. We'd just need to not
crash if that's the case.
value: {
"status": "setup",
"socket-address": [
{
"port": "46213",
"ipv4": true,
"host": "127.0.0.1",
"type": "inet"
}
]
}
Okay, I understood your ask here. This is what gets printed from the QDict.
Let me raise a patch to return with a message if the QDict does not have key
with 'socket-address'. This would prevent the crash later on.
I wanted to know what other than "socket-address" key can he QDict give us
because, if that's the case, for other than socket migration, then we can
make this function more generic rather than having it as
'migrate_get_socket_address'
For now, there's nothing else. Let's just ignore when socket-address is
missing in the reply so we don't break future tests that use a
non-socket type.
Okay, Done. Can find the build here:
https://gitlab.com/galahet/Qemu/-/pipelines/1219841944
I only noticed this because I was fiddling with the file migration API
and this series helped me a lot to test my changes. So thanks for that,
Het.
Another point is: we really need to encourage people to write tests
using the new channels API. I added the FileMigrationArgs with the
'offset' as a required parameter, not even knowing optional parameters
were a thing. So it's obviously not enough to write support for the new
API if no tests ever touch it.
Yes, definitely we need more tests with the new channels API to test other
than just tcp connection. I could give a try for vsock and unix with the
new QAPI syntax, and add some tests.
I also wanted to bring in attention that, this solution I what i feel is
also
not complete. If we are using new channel syntax for migrate_qmp, then the
same syntax should also be used for migrate_qmp_incoming. But we haven't
touch that, and it still prints the old syntax. We might need a lot changes
in design maybe to incorporate that too in new tests totally with the new
syntax.
Adding migrate_qmp_incoming support should be relatively simple. You had
patches for that in another version, no?
No Fabiano, what I meant was, in migration-test.c, change in
migrate_incoming_qmp
would need to change the callback function and ultimately change all the
callback
handlers ? In that sense, it would require a big change ?
Inside the migrate_incoming_qmp function, adding implementation for
channels is
same as other migrate_* function.
You could add the parameter to migrate_incoming_qmp and use NULL when
calling. The callbacks don't need to be changed. When we add more tests
then we'd alter the callbacks accordingly.
I might convert the file tests soon, you can leave that part to me if
you want.
Okay, sure. Will leave those changes to you.
Another thing that you also noted down while discussing on the patches that
we should have a standard pattern on how to define the migration tests. Even
that would be helpful for the users, on how to add new tests, where to add
new tests in the file, and which test is needed to run if a specific change
needs to be tested.
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
Regards,
Het Gala
Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
FYI: This 2 patches are rebased on top of another
(tests/qtest/migration: Add tests for introducing 'channels' argument in
migrate QAPIs) series.
Can find the build for both the patches here:
https://gitlab.com/galahet/Qemu/-/pipelines/1219841944
On 20/03/24 2:18 am, Het Gala wrote:
'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.
Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.
Suggested-by: Fabiano Rosas
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..fb7156f09a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState
*who)
QObject *object;
rsp = migrate_query(who);
+
+if (!qdict_haskey(rsp, "socket-address")) {
+return NULL;
+}
object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
Regards,
Het Gala
Re: [PATCH v5 15/24] tests/avocado: reverse_debugging.py add test for x86-64 q35 machine
Nicholas Piggin writes: > The x86-64 pc machine has a problem with record/replay. q35 seems > to work well. Add a new q35 test and update the flaky message for > pc. > > Signed-off-by: Nicholas Piggin Reviewed-by: Alex Bennée Tested-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH v5 12/24] savevm: Fix load_snapshot error path crash
Nicholas Piggin writes: > An error path missed setting *errp, which can cause a NULL deref. > > Signed-off-by: Nicholas Piggin Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
[PATCH 2/2] tests/qtest/migration: Fix typo for vsock in SocketAddress_to_str
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index fb7156f09a..651c6c555a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -42,7 +42,7 @@ static char *SocketAddress_to_str(SocketAddress *addr)
case SOCKET_ADDRESS_TYPE_FD:
return g_strdup_printf("fd:%s", addr->u.fd.str);
case SOCKET_ADDRESS_TYPE_VSOCK:
-return g_strdup_printf("tcp:%s:%s",
+return g_strdup_printf("vsock:%s:%s",
addr->u.vsock.cid,
addr->u.vsock.port);
default:
--
2.22.3
[PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.
Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.
Suggested-by: Fabiano Rosas
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..fb7156f09a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState
*who)
QObject *object;
rsp = migrate_query(who);
+
+if (!qdict_haskey(rsp, "socket-address")) {
+return NULL;
+}
object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
--
2.22.3
Re: [PATCH v5 10/24] virtio-net: Use replay_schedule_bh_event for bhs that affect machine state
Nicholas Piggin writes: > The regular qemu_bh_schedule() calls result in non-deterministic > execution of the bh in record-replay mode, which causes replay failure. > > Signed-off-by: Nicholas Piggin Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH v5 08/24] replay: Fix migration use of clock
Nicholas Piggin writes:
> Migration reads host clocks when not holding the replay_mutex, which
> asserts when recording a trace. It seems that these migration times
> should be host times like other statistics in MigrationState.
s/host/CLOCK_HOST/ and s/host/CLOCK_REALTIME/ but its a confusing
sentence. If the MigrationState is guest visible it should be
QEMU_CLOCK_VIRTUAL surely?
> These
> do not require the replay_mutex.
>
> Signed-off-by: Nicholas Piggin
> ---
> migration/migration.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 644e073b7d..2c286ccf63 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3424,7 +3424,7 @@ static void *migration_thread(void *opaque)
> {
> MigrationState *s = opaque;
> MigrationThread *thread = NULL;
> -int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> MigThrError thr_error;
> bool urgent = false;
>
> @@ -3476,7 +3476,7 @@ static void *migration_thread(void *opaque)
> qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> MIGRATION_STATUS_ACTIVE);
>
> -s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - setup_start;
>
> trace_migration_thread_setup_complete();
>
> @@ -3555,7 +3555,7 @@ static void *bg_migration_thread(void *opaque)
>
> migration_rate_set(RATE_LIMIT_DISABLED);
>
> -setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +setup_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> /*
> * We want to save vmstate for the moment when migration has been
> * initiated but also we want to save RAM content while VM is running.
> @@ -3588,7 +3588,7 @@ static void *bg_migration_thread(void *opaque)
> qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> MIGRATION_STATUS_ACTIVE);
>
> -s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - setup_start;
>
> trace_migration_thread_setup_complete();
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
[PATCH] Implement SSH commands in QEMU GA for Windows
From: aidaleuc
Signed-off-by: aidaleuc
---
qga/commands-posix-ssh.c | 47 +--
qga/commands-ssh-core.c| 52 +++
qga/commands-ssh-core.h| 20 ++
qga/commands-windows-ssh.c | 686 +
qga/meson.build| 6 +-
qga/qapi-schema.json | 20 +-
6 files changed, 775 insertions(+), 56 deletions(-)
create mode 100644 qga/commands-ssh-core.c
create mode 100644 qga/commands-ssh-core.h
create mode 100644 qga/commands-windows-ssh.c
diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
index 236f80de44..7058894007 100644
--- a/qga/commands-posix-ssh.c
+++ b/qga/commands-posix-ssh.c
@@ -11,6 +11,7 @@
#include "qapi/error.h"
#include "qga-qapi-commands.h"
+#include "commands-ssh-core.h"
#ifdef QGA_BUILD_UNIT_TEST
static struct passwd *
@@ -80,37 +81,6 @@ mkdir_for_user(const char *path, const struct passwd *p,
return true;
}
-static bool
-check_openssh_pub_key(const char *key, Error **errp)
-{
-/* simple sanity-check, we may want more? */
-if (!key || key[0] == '#' || strchr(key, '\n')) {
-error_setg(errp, "invalid OpenSSH public key: '%s'", key);
-return false;
-}
-
-return true;
-}
-
-static bool
-check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
-{
-size_t n = 0;
-strList *k;
-
-for (k = keys; k != NULL; k = k->next) {
-if (!check_openssh_pub_key(k->value, errp)) {
-return false;
-}
-n++;
-}
-
-if (nkeys) {
-*nkeys = n;
-}
-return true;
-}
-
static bool
write_authkeys(const char *path, const GStrv keys,
const struct passwd *p, Error **errp)
@@ -139,21 +109,6 @@ write_authkeys(const char *path, const GStrv keys,
return true;
}
-static GStrv
-read_authkeys(const char *path, Error **errp)
-{
-g_autoptr(GError) err = NULL;
-g_autofree char *contents = NULL;
-
-if (!g_file_get_contents(path, , NULL, )) {
-error_setg(errp, "failed to read '%s': %s", path, err->message);
-return NULL;
-}
-
-return g_strsplit(contents, "\n", -1);
-
-}
-
void
qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
bool has_reset, bool reset,
diff --git a/qga/commands-ssh-core.c b/qga/commands-ssh-core.c
new file mode 100644
index 00..2c9fca791a
--- /dev/null
+++ b/qga/commands-ssh-core.c
@@ -0,0 +1,52 @@
+#include "qemu/osdep.h"
+#include
+#include
+#include "qapi/error.h"
+#include "commands-ssh-core.h"
+
+GStrv read_authkeys(const char *path, Error **errp)
+{
+g_autoptr(GError) err = NULL;
+g_autofree char *contents = NULL;
+
+if (!g_file_get_contents(path, , NULL, ))
+{
+error_setg(errp, "failed to read '%s': %s", path, err->message);
+return NULL;
+}
+
+return g_strsplit(contents, "\n", -1);
+}
+
+bool check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
+{
+size_t n = 0;
+strList *k;
+
+for (k = keys; k != NULL; k = k->next)
+{
+if (!check_openssh_pub_key(k->value, errp))
+{
+return false;
+}
+n++;
+}
+
+if (nkeys)
+{
+*nkeys = n;
+}
+return true;
+}
+
+bool check_openssh_pub_key(const char *key, Error **errp)
+{
+/* simple sanity-check, we may want more? */
+if (!key || key[0] == '#' || strchr(key, '\n'))
+{
+error_setg(errp, "invalid OpenSSH public key: '%s'", key);
+return false;
+}
+
+return true;
+}
\ No newline at end of file
diff --git a/qga/commands-ssh-core.h b/qga/commands-ssh-core.h
new file mode 100644
index 00..b6866cff22
--- /dev/null
+++ b/qga/commands-ssh-core.h
@@ -0,0 +1,20 @@
+#include
+#include "qemu/osdep.h"
+
+GStrv read_authkeys(const char *path, Error **errp);
+bool check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp);
+bool check_openssh_pub_key(const char *key, Error **errp);
+
+typedef struct WindowsUserInfo
+{
+char *sshDirectory;
+char *authorizedKeyFile;
+char *username;
+char *SSID;
+bool isAdmin;
+} WindowsUserInfo;
+
+typedef WindowsUserInfo *PWindowsUserInfo;
+
+void free_userInfo(PWindowsUserInfo info);
+G_DEFINE_AUTO_CLEANUP_FREE_FUNC(PWindowsUserInfo, free_userInfo, NULL);
\ No newline at end of file
diff --git a/qga/commands-windows-ssh.c b/qga/commands-windows-ssh.c
new file mode 100644
index 00..59b9309eff
--- /dev/null
+++ b/qga/commands-windows-ssh.c
@@ -0,0 +1,686 @@
+/*
+ * QEMU Guest Agent win32-specific command implementations for SSH keys.
+ * The implementation is opinionated and expects the SSH implementation to
+ * be OpenSSH.
+ *
+ * Copyright IBM Corp. 2024
+ *
+ * Authors:
+ * Aidan Leuck
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include
+#include
+#include "qapi/error.h"
+#include
[PULL 6/8] crypto: query gcrypt for cipher availability
Just because a cipher is defined in the gcrypt header file, does not
imply that it can be used. Distros can filter the list of ciphers when
building gcrypt. For example, RHEL-9 disables the SM4 cipher. It is
also possible that running in FIPS mode might dynamically change what
ciphers are available at runtime.
qcrypto_cipher_supports must therefore query gcrypt directly to check
for cipher availability.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Signed-off-by: Daniel P. Berrangé
---
crypto/cipher-gcrypt.c.inc | 5 +
1 file changed, 5 insertions(+)
diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc
index 6b82280f90..4a8314746d 100644
--- a/crypto/cipher-gcrypt.c.inc
+++ b/crypto/cipher-gcrypt.c.inc
@@ -93,6 +93,11 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
return false;
}
+if (gcry_cipher_algo_info(qcrypto_cipher_alg_to_gcry_alg(alg),
+ GCRYCTL_TEST_ALGO, NULL, NULL) != 0) {
+return false;
+}
+
switch (mode) {
case QCRYPTO_CIPHER_MODE_ECB:
case QCRYPTO_CIPHER_MODE_CBC:
--
2.43.0
[PULL 8/8] crypto: report which ciphers are being skipped during tests
Since the ciphers can be dynamically disabled at runtime, when running
unit tests it is helpful to report which ciphers we can skipped for
testing.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Signed-off-by: Daniel P. Berrangé
---
tests/unit/test-crypto-cipher.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/unit/test-crypto-cipher.c b/tests/unit/test-crypto-cipher.c
index d0ea7b4d8e..f5152e569d 100644
--- a/tests/unit/test-crypto-cipher.c
+++ b/tests/unit/test-crypto-cipher.c
@@ -821,6 +821,10 @@ int main(int argc, char **argv)
for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
if (qcrypto_cipher_supports(test_data[i].alg, test_data[i].mode)) {
g_test_add_data_func(test_data[i].path, _data[i],
test_cipher);
+} else {
+g_printerr("# skip unsupported %s:%s\n",
+ QCryptoCipherAlgorithm_str(test_data[i].alg),
+ QCryptoCipherMode_str(test_data[i].mode));
}
}
--
2.43.0
[PULL 2/8] chardev: lower priority of the HUP GSource in socket chardev
The socket chardev often has 2 GSource object registered against the same FD. One is registered all the time and is just intended to handle POLLHUP events, while the other gets registered & unregistered on the fly as the frontend is ready to receive more data or not. It is very common for poll() to signal a POLLHUP event at the same time as there is pending incoming data from the disconnected client. It is therefore essential to process incoming data prior to processing HUP. The problem with having 2 GSource on the same FD is that there is no guaranteed ordering of execution between them, so the chardev code may process HUP first and thus discard data. This failure scenario is non-deterministic but can be seen fairly reliably by reverting a7077b8e354d90fec26c2921aa2dea85b90dff90, and then running 'tests/unit/test-char', which will sometimes fail with missing data. Ideally QEMU would only have 1 GSource, but that's a complex code refactoring job. The next best solution is to try to ensure ordering between the 2 GSource objects. This can be achieved by lowering the priority of the HUP GSource, so that it is never dispatched if the main GSource is also ready to dispatch. Counter-intuitively, lowering the priority of a GSource is done by raising its priority number. Reviewed-by: Marc-André Lureau Reviewed-by: Thomas Huth Signed-off-by: Daniel P. Berrangé --- chardev/char-socket.c | 16 1 file changed, 16 insertions(+) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 8a0406cc1e..2c4dffc0e6 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -601,6 +601,22 @@ static void update_ioc_handlers(SocketChardev *s) remove_hup_source(s); s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP); +/* + * poll() is liable to return POLLHUP even when there is + * still incoming data available to read on the FD. If + * we have the hup_source at the same priority as the + * main io_add_watch_poll GSource, then we might end up + * processing the POLLHUP event first, closing the FD, + * and as a result silently discard data we should have + * read. + * + * By setting the hup_source to G_PRIORITY_DEFAULT + 1, + * we ensure that io_add_watch_poll GSource will always + * be dispatched first, thus guaranteeing we will be + * able to process all incoming data before closing the + * FD + */ +g_source_set_priority(s->hup_source, G_PRIORITY_DEFAULT + 1); g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup, chr, NULL); g_source_attach(s->hup_source, chr->gcontext); -- 2.43.0
[PULL 4/8] Revert "chardev: use a child source for qio input source"
This reverts commit a7077b8e354d90fec26c2921aa2dea85b90dff90,
and add comments to explain why child sources cannot be used.
When a GSource is added as a child of another GSource, if its
'prepare' function indicates readiness, then the parent's
'prepare' function will never be run. The io_watch_poll_prepare
absolutely *must* be run on every iteration of the main loop,
to ensure that the chardev backend doesn't feed data to the
frontend that it is unable to consume.
At the time a7077b8e354d90fec26c2921aa2dea85b90dff90 was made,
all the child GSource impls were relying on poll'ing an FD,
so their 'prepare' functions would never indicate readiness
ahead of poll() being invoked. So the buggy behaviour was
not noticed and lay dormant.
Relatively recently the QIOChannelTLS impl introduced a
level 2 child GSource, which checks with GNUTLS whether it
has cached any data that was decoded but not yet consumed:
commit ffda5db65aef42266a5053a4be34515106c4c7ee
Author: Antoine Damhet
Date: Tue Nov 15 15:23:29 2022 +0100
io/channel-tls: fix handling of bigger read buffers
Since the TLS backend can read more data from the underlying QIOChannel
we introduce a minimal child GSource to notify if we still have more
data available to be read.
Signed-off-by: Antoine Damhet
Signed-off-by: Charles Frey
Signed-off-by: Daniel P. Berrangé
With this, it is now quite common for the 'prepare' function
on a QIOChannelTLS GSource to indicate immediate readiness,
bypassing the parent GSource 'prepare' function. IOW, the
critical 'io_watch_poll_prepare' is being skipped on some
iterations of the main loop. As a result chardev frontend
asserts are now being triggered as they are fed data they
are not ready to consume.
A reproducer is as follows:
* In terminal 1 run a GNUTLS *echo* server
$ gnutls-serv --echo \
--x509cafile ca-cert.pem \
--x509keyfile server-key.pem \
--x509certfile server-cert.pem \
-p 9000
* In terminal 2 run a QEMU guest
$ qemu-system-s390x \
-nodefaults \
-display none \
-object tls-creds-x509,id=tls0,dir=$PWD,endpoint=client \
-chardev socket,id=con0,host=localhost,port=9000,tls-creds=tls0 \
-device sclpconsole,chardev=con0 \
-hda Fedora-Cloud-Base-39-1.5.s390x.qcow2
After the previous patch revert, but before this patch revert,
this scenario will crash:
qemu-system-s390x: ../hw/char/sclpconsole.c:73: chr_read: Assertion
`size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed.
This assert indicates that 'tcp_chr_read' was called without
'tcp_chr_read_poll' having first been checked for ability to
receive more data
QEMU's use of a 'prepare' function to create/delete another
GSource is rather a hack and not normally the kind of thing that
is expected to be done by a GSource. There is no mechanism to
force GLib to always run the 'prepare' function of a parent
GSource. The best option is to simply not use the child source
concept, and go back to the functional approach previously
relied on.
Reviewed-by: Marc-André Lureau
Reviewed-by: Thomas Huth
Tested-by: Thomas Huth
Signed-off-by: Daniel P. Berrangé
---
chardev/char-io.c | 56 ++-
1 file changed, 51 insertions(+), 5 deletions(-)
diff --git a/chardev/char-io.c b/chardev/char-io.c
index 4451128cba..dab77b112e 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -33,6 +33,7 @@ typedef struct IOWatchPoll {
IOCanReadHandler *fd_can_read;
GSourceFunc fd_read;
void *opaque;
+GMainContext *context;
} IOWatchPoll;
static IOWatchPoll *io_watch_poll_from_source(GSource *source)
@@ -50,28 +51,59 @@ static gboolean io_watch_poll_prepare(GSource *source,
return FALSE;
}
+/*
+ * We do not register the QIOChannel watch as a child GSource.
+ * The 'prepare' function on the parent GSource will be
+ * skipped if a child GSource's 'prepare' function indicates
+ * readiness. We need this prepare function be guaranteed
+ * to run on *every* iteration of the main loop, because
+ * it is critical to ensure we remove the QIOChannel watch
+ * if 'fd_can_read' indicates the frontend cannot receive
+ * more data.
+ */
if (now_active) {
iwp->src = qio_channel_create_watch(
iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
-g_source_add_child_source(source, iwp->src);
-g_source_unref(iwp->src);
+g_source_attach(iwp->src, iwp->context);
} else {
-g_source_remove_child_source(source, iwp->src);
+g_source_destroy(iwp->src);
+g_source_unref(iwp->src);
iwp->src = NULL;
}
return FALSE;
}
+static gboolean io_watch_poll_check(GSource *source)
+{
+return FALSE;
+}
+
static gboolean io_watch_poll_dispatch(GSource
[PULL 7/8] crypto: use error_abort for unexpected failures
This improves the error diagnosis from the unit test when a cipher
is unexpected not available from
ERROR:../tests/unit/test-crypto-cipher.c:683:test_cipher: assertion failed:
(err == NULL)
Bail out! ERROR:../tests/unit/test-crypto-cipher.c:683:test_cipher: assertion
failed: (err == NULL)
Aborted (core dumped)
to
Unexpected error in qcrypto_cipher_ctx_new() at
../crypto/cipher-gcrypt.c.inc:262:
./build//tests/unit/test-crypto-cipher: Cannot initialize cipher: Invalid
cipher algorithm
Aborted (core dumped)
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Signed-off-by: Daniel P. Berrangé
---
tests/unit/test-crypto-cipher.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/unit/test-crypto-cipher.c b/tests/unit/test-crypto-cipher.c
index 11ab1a54fc..d0ea7b4d8e 100644
--- a/tests/unit/test-crypto-cipher.c
+++ b/tests/unit/test-crypto-cipher.c
@@ -676,9 +676,8 @@ static void test_cipher(const void *opaque)
cipher = qcrypto_cipher_new(
data->alg, data->mode,
key, nkey,
-);
+data->plaintext ? _abort : );
if (data->plaintext) {
-g_assert(err == NULL);
g_assert(cipher != NULL);
} else {
error_free_or_abort();
--
2.43.0
[PULL 1/8] seccomp: report EPERM instead of killing process for spawn set
When something tries to run one of the spawn syscalls (eg clone),
our seccomp deny filter is set to cause a fatal trap which kills
the process.
This is found to be unhelpful when QEMU has loaded the nvidia
GL library. This tries to spawn a process to modprobe the nvidia
kmod. This is a dubious thing to do, but at the same time, the
code will gracefully continue if this fails. Our seccomp filter
rightly blocks the spawning, but prevent the graceful continue.
Switching to reporting EPERM will make QEMU behave more gracefully
without impacting the level of protect we have.
https://gitlab.com/qemu-project/qemu/-/issues/2116
Signed-off-by: Daniel P. Berrangé
---
system/qemu-seccomp.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/system/qemu-seccomp.c b/system/qemu-seccomp.c
index 4d7439e7f7..98ffce075c 100644
--- a/system/qemu-seccomp.c
+++ b/system/qemu-seccomp.c
@@ -74,7 +74,7 @@ const struct scmp_arg_cmp sched_setscheduler_arg[] = {
#define RULE_CLONE_FLAG(flag) \
{ SCMP_SYS(clone), QEMU_SECCOMP_SET_SPAWN, \
- ARRAY_SIZE(clone_arg ## flag), clone_arg ## flag, SCMP_ACT_TRAP }
+ ARRAY_SIZE(clone_arg ## flag), clone_arg ## flag, SCMP_ACT_ERRNO(EPERM) }
/* If no CLONE_* flags are set, except CSIGNAL, deny */
const struct scmp_arg_cmp clone_arg_none[] = {
@@ -214,13 +214,13 @@ static const struct QemuSeccompSyscall denylist[] = {
0, NULL, SCMP_ACT_TRAP },
/* spawn */
{ SCMP_SYS(fork), QEMU_SECCOMP_SET_SPAWN,
- 0, NULL, SCMP_ACT_TRAP },
+ 0, NULL, SCMP_ACT_ERRNO(EPERM) },
{ SCMP_SYS(vfork), QEMU_SECCOMP_SET_SPAWN,
- 0, NULL, SCMP_ACT_TRAP },
+ 0, NULL, SCMP_ACT_ERRNO(EPERM) },
{ SCMP_SYS(execve), QEMU_SECCOMP_SET_SPAWN,
- 0, NULL, SCMP_ACT_TRAP },
+ 0, NULL, SCMP_ACT_ERRNO(EPERM) },
{ SCMP_SYS(clone), QEMU_SECCOMP_SET_SPAWN,
- ARRAY_SIZE(clone_arg_none), clone_arg_none, SCMP_ACT_TRAP },
+ ARRAY_SIZE(clone_arg_none), clone_arg_none, SCMP_ACT_ERRNO(EPERM) },
RULE_CLONE_FLAG(CLONE_VM),
RULE_CLONE_FLAG(CLONE_FS),
RULE_CLONE_FLAG(CLONE_FILES),
--
2.43.0
[PULL 3/8] Revert "chardev/char-socket: Fix TLS io channels sending too much data to the backend"
This commit results in unexpected termination of the TLS connection.
When 'fd_can_read' returns 0, the code goes on to pass a zero length
buffer to qio_channel_read. The TLS impl calls into gnutls_recv()
with this zero length buffer, at which point GNUTLS returns an error
GNUTLS_E_INVALID_REQUEST. This is treated as fatal by QEMU's TLS code
resulting in the connection being torn down by the chardev.
Simply skipping the qio_channel_read when the buffer length is zero
is also not satisfactory, as it results in a high CPU burn busy loop
massively slowing QEMU's functionality.
The proper solution is to avoid tcp_chr_read being called at all
unless the frontend is able to accept more data. This will be done
in a followup commit.
This reverts commit 462945cd22d2bcd233401ed3aa167d83a8e35b05
Reviewed-by: Thomas Huth
Signed-off-by: Daniel P. Berrangé
---
chardev/char-socket.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 2c4dffc0e6..812d7aa38a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -496,9 +496,9 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition
cond, void *opaque)
s->max_size <= 0) {
return TRUE;
}
-len = tcp_chr_read_poll(opaque);
-if (len > sizeof(buf)) {
-len = sizeof(buf);
+len = sizeof(buf);
+if (len > s->max_size) {
+len = s->max_size;
}
size = tcp_chr_recv(chr, (void *)buf, len);
if (size == 0 || (size == -1 && errno != EAGAIN)) {
--
2.43.0
[PULL 0/8] Misc fixes patches
The following changes since commit c62d54d0a8067ffb3d5b909276f7296d7df33fa7: Update version for v9.0.0-rc0 release (2024-03-19 19:13:52 +) are available in the Git repository at: https://gitlab.com/berrange/qemu tags/misc-fixes-pull-request for you to fetch changes up to c3b1aa1c1ae66e0174704072b1fb7d10d6e4a4b7: crypto: report which ciphers are being skipped during tests (2024-03-19 20:17:12 +) * Use EPERM for seccomp filter instead of killing QEMU when an attempt to spawn child process is made * Reduce priority of POLLHUP handling for socket chardevs to increase likelihood of pending data being processed * Fix chardev I/O main loop integration when TLS is enabled * Fix broken crypto test suite when distro disables SM4 algorithm * Improve diagnosis of failed crypto tests Daniel P. Berrangé (8): seccomp: report EPERM instead of killing process for spawn set chardev: lower priority of the HUP GSource in socket chardev Revert "chardev/char-socket: Fix TLS io channels sending too much data to the backend" Revert "chardev: use a child source for qio input source" crypto: factor out conversion of QAPI to gcrypt constants crypto: query gcrypt for cipher availability crypto: use error_abort for unexpected failures crypto: report which ciphers are being skipped during tests chardev/char-io.c | 56 +-- chardev/char-socket.c | 22 +- crypto/cipher-gcrypt.c.inc | 121 +--- system/qemu-seccomp.c | 10 +-- tests/unit/test-crypto-cipher.c | 7 +- 5 files changed, 145 insertions(+), 71 deletions(-) -- 2.43.0
[PULL 5/8] crypto: factor out conversion of QAPI to gcrypt constants
The conversion of cipher mode will shortly be required in more
than one place.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Signed-off-by: Daniel P. Berrangé
---
crypto/cipher-gcrypt.c.inc | 116 +++--
1 file changed, 60 insertions(+), 56 deletions(-)
diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc
index 1377cbaf14..6b82280f90 100644
--- a/crypto/cipher-gcrypt.c.inc
+++ b/crypto/cipher-gcrypt.c.inc
@@ -20,6 +20,56 @@
#include
+static int qcrypto_cipher_alg_to_gcry_alg(QCryptoCipherAlgorithm alg)
+{
+switch (alg) {
+case QCRYPTO_CIPHER_ALG_DES:
+return GCRY_CIPHER_DES;
+case QCRYPTO_CIPHER_ALG_3DES:
+return GCRY_CIPHER_3DES;
+case QCRYPTO_CIPHER_ALG_AES_128:
+return GCRY_CIPHER_AES128;
+case QCRYPTO_CIPHER_ALG_AES_192:
+return GCRY_CIPHER_AES192;
+case QCRYPTO_CIPHER_ALG_AES_256:
+return GCRY_CIPHER_AES256;
+case QCRYPTO_CIPHER_ALG_CAST5_128:
+return GCRY_CIPHER_CAST5;
+case QCRYPTO_CIPHER_ALG_SERPENT_128:
+return GCRY_CIPHER_SERPENT128;
+case QCRYPTO_CIPHER_ALG_SERPENT_192:
+return GCRY_CIPHER_SERPENT192;
+case QCRYPTO_CIPHER_ALG_SERPENT_256:
+return GCRY_CIPHER_SERPENT256;
+case QCRYPTO_CIPHER_ALG_TWOFISH_128:
+return GCRY_CIPHER_TWOFISH128;
+case QCRYPTO_CIPHER_ALG_TWOFISH_256:
+return GCRY_CIPHER_TWOFISH;
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+return GCRY_CIPHER_SM4;
+#endif
+default:
+return GCRY_CIPHER_NONE;
+}
+}
+
+static int qcrypto_cipher_mode_to_gcry_mode(QCryptoCipherMode mode)
+{
+switch (mode) {
+case QCRYPTO_CIPHER_MODE_ECB:
+return GCRY_CIPHER_MODE_ECB;
+case QCRYPTO_CIPHER_MODE_XTS:
+return GCRY_CIPHER_MODE_XTS;
+case QCRYPTO_CIPHER_MODE_CBC:
+return GCRY_CIPHER_MODE_CBC;
+case QCRYPTO_CIPHER_MODE_CTR:
+return GCRY_CIPHER_MODE_CTR;
+default:
+return GCRY_CIPHER_MODE_NONE;
+}
+}
+
bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
QCryptoCipherMode mode)
{
@@ -188,72 +238,26 @@ static QCryptoCipher
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
return NULL;
}
-switch (alg) {
-case QCRYPTO_CIPHER_ALG_DES:
-gcryalg = GCRY_CIPHER_DES;
-break;
-case QCRYPTO_CIPHER_ALG_3DES:
-gcryalg = GCRY_CIPHER_3DES;
-break;
-case QCRYPTO_CIPHER_ALG_AES_128:
-gcryalg = GCRY_CIPHER_AES128;
-break;
-case QCRYPTO_CIPHER_ALG_AES_192:
-gcryalg = GCRY_CIPHER_AES192;
-break;
-case QCRYPTO_CIPHER_ALG_AES_256:
-gcryalg = GCRY_CIPHER_AES256;
-break;
-case QCRYPTO_CIPHER_ALG_CAST5_128:
-gcryalg = GCRY_CIPHER_CAST5;
-break;
-case QCRYPTO_CIPHER_ALG_SERPENT_128:
-gcryalg = GCRY_CIPHER_SERPENT128;
-break;
-case QCRYPTO_CIPHER_ALG_SERPENT_192:
-gcryalg = GCRY_CIPHER_SERPENT192;
-break;
-case QCRYPTO_CIPHER_ALG_SERPENT_256:
-gcryalg = GCRY_CIPHER_SERPENT256;
-break;
-case QCRYPTO_CIPHER_ALG_TWOFISH_128:
-gcryalg = GCRY_CIPHER_TWOFISH128;
-break;
-case QCRYPTO_CIPHER_ALG_TWOFISH_256:
-gcryalg = GCRY_CIPHER_TWOFISH;
-break;
-#ifdef CONFIG_CRYPTO_SM4
-case QCRYPTO_CIPHER_ALG_SM4:
-gcryalg = GCRY_CIPHER_SM4;
-break;
-#endif
-default:
+gcryalg = qcrypto_cipher_alg_to_gcry_alg(alg);
+if (gcryalg == GCRY_CIPHER_NONE) {
error_setg(errp, "Unsupported cipher algorithm %s",
QCryptoCipherAlgorithm_str(alg));
return NULL;
}
-drv = _gcrypt_driver;
-switch (mode) {
-case QCRYPTO_CIPHER_MODE_ECB:
-gcrymode = GCRY_CIPHER_MODE_ECB;
-break;
-case QCRYPTO_CIPHER_MODE_XTS:
-gcrymode = GCRY_CIPHER_MODE_XTS;
-break;
-case QCRYPTO_CIPHER_MODE_CBC:
-gcrymode = GCRY_CIPHER_MODE_CBC;
-break;
-case QCRYPTO_CIPHER_MODE_CTR:
-drv = _gcrypt_ctr_driver;
-gcrymode = GCRY_CIPHER_MODE_CTR;
-break;
-default:
+gcrymode = qcrypto_cipher_mode_to_gcry_mode(mode);
+if (gcrymode == GCRY_CIPHER_MODE_NONE) {
error_setg(errp, "Unsupported cipher mode %s",
QCryptoCipherMode_str(mode));
return NULL;
}
+if (mode == QCRYPTO_CIPHER_MODE_CTR) {
+drv = _gcrypt_ctr_driver;
+} else {
+drv = _gcrypt_driver;
+}
+
ctx = g_new0(QCryptoCipherGcrypt, 1);
ctx->base.driver = drv;
--
2.43.0
Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs
On Tue, Mar 19, 2024 at 07:52:47PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 19, 2024 at 03:25:18PM -0400, Peter Xu wrote:
> > On Tue, Mar 19, 2024 at 04:25:32PM +, Daniel P. Berrangé wrote:
> > > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote:
> > > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> > > > > Peter Xu writes:
> > > > >
> > > > > > [I queued patch 1-2 into -stable, leaving this patch for further
> > > > > > discussions]
> > > > > >
> > > > > > On Fri, Mar 15, 2024 at 08:55:42AM +, Daniel P. Berrangé wrote:
> > > > > >> The 'file:' protocol eventually calls into qemu_open, and this
> > > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax
> > > > > >> to pass in FDs.
> > > > > >
> > > > > > If it always use /dev/fdsets for files, does it mean that the newly
> > > > > > added
> > > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so
> > > > > > we can
> > > > > > drop them)?
> > > > >
> > > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> > > > > MigrationAddress was added. So this:
> > > > >
> > > > > 'channels': [ { 'channel-type': 'main',
> > > > > 'addr': { 'transport': 'socket',
> > > > > 'type': 'fd',
> > > > > 'str': 'fdname' } } ]
> > > > >
> > > > > works without multifd and without mapped-ram if the fd is a file or
> > > > > socket.
> > > > >
> > > > > So yes, you're correct, but given we already have this^ it would be
> > > > > perhaps more confusing for users to allow it, but not allow the very
> > > > > same JSON when multifd=true, mapped-ram=true.
> > > >
> > > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON
> > > > format) is trivial to use. If libvirt didn't use it I won't be
> > > > surprised to
> > > > see nobody using it. I want us to avoid working on things that nobody
> > > > is
> > > > using, or has a better replacement.
> > > >
> > > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/
> > > > works
> > > > for all the cases that libvirt needs. I am aware that the old getfd has
> > > > the monitor limitation so that if the QMP disconnected and reconnect,
> > > > the
> > > > fd can be gone. However I'm not sure whether that's the only reason to
> > > > have add-fd, and also not sure whether it means add-fd is always
> > > > preferred,
> > > > so that maybe we can consider obsolete getfd?
> > >
> > > Historically libvirt primariily uses the 'fd:' protocol, with a
> > > socket FD. It never gives QEMU a plain file FD, since it has
> > > always added its "iohelper" as a MITM, in order to add O_DIRECT
> > > on top.
> > >
> > > The 'getfd' command is something that is needed when talking to
> > > QEMU for any API that involves a "SocketAddress" QAPI type,
> > > which is applicable for migration.
> > >
> > > With the introduction of 'MigrationAddress', the 'socket' protocol
> > > is backed by 'SocketAddress' and thus supports FD passing for
> > > sockets (or potentally pipes too), in combination with 'getfd'.
> > >
> > > With the 'file' protocol in 'MigrationAddress', since it gets
> > > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide
> > > passing for plain files.
> >
> > I see. I assume it means we still have multiple users of getfd so it's
> > still in use where add-fd is not yet avaiable.
> >
> > But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole
> > mapped-ram effort, neither do we need any support on file migrations over
> > "fd", e.g. fd_start_incoming_migration() for files. So we can drop these
> > parts, am I right?
>
> Correct, libvirt hasn't got any impl for 'mapped-ram' yet, at least
> not something merged.
>
> Since this is new functionality, libvirt could go straight for the
> 'file' protocol / address type.
>
> At some point I think we can stop using 'fd' for traditional migration
> too and pass the socket address to QEMU and let QEMU open the socket.
Thanks for confirming this, that sounds good. I quickly discussed this
with Fabiano just now, I think there's a plan we start to mark fd migration
deprecated for the next release (9.1), then there is a chance we drop it in
migration for 9.3. That'll copy libvirt list so we can re-check there.
Fabiano will then prepare patches to remove the "fd:" support on file
migrations; that will be for 9.0.
Thanks,
--
Peter Xu
Re: [PATCH] coroutine: cap per-thread local pool size
On Tue, Mar 19, 2024 at 06:41:28PM +0100, Kevin Wolf wrote:
> Am 19.03.2024 um 18:10 hat Daniel P. Berrangé geschrieben:
> > On Tue, Mar 19, 2024 at 05:54:38PM +0100, Kevin Wolf wrote:
> > > Am 19.03.2024 um 14:43 hat Daniel P. Berrangé geschrieben:
> > > > On Mon, Mar 18, 2024 at 02:34:29PM -0400, Stefan Hajnoczi wrote:
> > > > > The coroutine pool implementation can hit the Linux vm.max_map_count
> > > > > limit, causing QEMU to abort with "failed to allocate memory for
> > > > > stack"
> > > > > or "failed to set up stack guard page" during coroutine creation.
> > > > >
> > > > > This happens because per-thread pools can grow to tens of thousands of
> > > > > coroutines. Each coroutine causes 2 virtual memory areas to be
> > > > > created.
> > > >
> > > > This sounds quite alarming. What usage scenario is justified in
> > > > creating so many coroutines?
> > >
> > > Basically we try to allow pooling coroutines for as many requests as
> > > there can be in flight at the same time. That is, adding a virtio-blk
> > > device increases the maximum pool size by num_queues * queue_size. If
> > > you have a guest with many CPUs, the default num_queues is relatively
> > > large (the bug referenced by Stefan had 64), and queue_size is 256 by
> > > default. That's 16k potential requests in flight per disk.
> >
> > If we have more than 1 virtio-blk device, does that scale up the max
> > coroutines too ?
> >
> > eg would 32 virtio-blks devices imply 16k * 32 -> 512k potential
> > requests/coroutines ?
>
> Yes. This is the number of request descriptors that fit in the
> virtqueues, and if you add another device with additional virtqueues,
> then obviously that increases the number of theoretically possible
> parallel requests.
>
> The limits of what you can actually achieve in practice might be lower
> because I/O might complete faster than the time we need to process all
> of the queued requests, depending on how many vcpus are trying to
> "compete" with how many iothreads. Of course, the practical limits in
> five years might be different from today.
>
> > > > IIUC, coroutine stack size is 1 MB, and so tens of thousands of
> > > > coroutines implies 10's of GB of memory just on stacks alone.
> > >
> > > That's only virtual memory, though. Not sure how much of it is actually
> > > used in practice.
> >
> > True, by default Linux wouldn't care too much about virtual memory,
> > Only if 'vm.overcommit_memory' is changed from its default, such
> > that Linux applies an overcommit ratio on RAM, then total virtual
> > memory would be relevant.
>
> That's a good point and one that I don't have a good answer for, short
> of just replacing the whole QEMU block layer with rsd and switching to
> stackless coroutines/futures this way.
>
> > > > > Eventually vm.max_map_count is reached and memory-related syscalls
> > > > > fail.
> > > >
> > > > On my system max_map_count is 1048576, quite alot higher than
> > > > 10's of 1000's. Hitting that would imply ~500,000 coroutines and
> > > > ~500 GB of stacks !
> > >
> > > Did you change the configuration some time in the past, or is this just
> > > a newer default? I get 65530, and that's the same default number I've
> > > seen in the bug reports.
> >
> > It turns out it is a Fedora change, rather than a kernel change:
> >
> > https://fedoraproject.org/wiki/Changes/IncreaseVmMaxMapCount
>
> Good to know, thanks.
>
> > > > > diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> > > > > index 5fd2dbaf8b..2790959eaf 100644
> > > > > --- a/util/qemu-coroutine.c
> > > > > +++ b/util/qemu-coroutine.c
> > > >
> > > > > +static unsigned int get_global_pool_hard_max_size(void)
> > > > > +{
> > > > > +#ifdef __linux__
> > > > > +g_autofree char *contents = NULL;
> > > > > +int max_map_count;
> > > > > +
> > > > > +/*
> > > > > + * Linux processes can have up to max_map_count virtual memory
> > > > > areas
> > > > > + * (VMAs). mmap(2), mprotect(2), etc fail with ENOMEM beyond
> > > > > this limit. We
> > > > > + * must limit the coroutine pool to a safe size to avoid running
> > > > > out of
> > > > > + * VMAs.
> > > > > + */
> > > > > +if (g_file_get_contents("/proc/sys/vm/max_map_count", ,
> > > > > NULL,
> > > > > +NULL) &&
> > > > > +qemu_strtoi(contents, NULL, 10, _map_count) == 0) {
> > > > > +/*
> > > > > + * This is a conservative upper bound that avoids exceeding
> > > > > + * max_map_count. Leave half for non-coroutine users like
> > > > > library
> > > > > + * dependencies, vhost-user, etc. Each coroutine takes up 2
> > > > > VMAs so
> > > > > + * halve the amount again.
> > > > > + */
> > > > > +return max_map_count / 4;
> > > >
> > > > That's 256,000 coroutines, which still sounds incredibly large
> > > > to me.
> > >
> > > The whole purpose of the limitation is that you won't ever get -ENOMEM
> > > back, which will
Re: [PATCH] coroutine: cap per-thread local pool size
On Tue, Mar 19, 2024 at 01:55:10PM -0400, Stefan Hajnoczi wrote:
> On Tue, Mar 19, 2024 at 01:43:32PM +, Daniel P. Berrangé wrote:
> > On Mon, Mar 18, 2024 at 02:34:29PM -0400, Stefan Hajnoczi wrote:
> > > diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> > > index 5fd2dbaf8b..2790959eaf 100644
> > > --- a/util/qemu-coroutine.c
> > > +++ b/util/qemu-coroutine.c
> >
> > > +static unsigned int get_global_pool_hard_max_size(void)
> > > +{
> > > +#ifdef __linux__
> > > +g_autofree char *contents = NULL;
> > > +int max_map_count;
> > > +
> > > +/*
> > > + * Linux processes can have up to max_map_count virtual memory areas
> > > + * (VMAs). mmap(2), mprotect(2), etc fail with ENOMEM beyond this
> > > limit. We
> > > + * must limit the coroutine pool to a safe size to avoid running out
> > > of
> > > + * VMAs.
> > > + */
> > > +if (g_file_get_contents("/proc/sys/vm/max_map_count", ,
> > > NULL,
> > > +NULL) &&
> > > +qemu_strtoi(contents, NULL, 10, _map_count) == 0) {
> > > +/*
> > > + * This is a conservative upper bound that avoids exceeding
> > > + * max_map_count. Leave half for non-coroutine users like library
> > > + * dependencies, vhost-user, etc. Each coroutine takes up 2 VMAs
> > > so
> > > + * halve the amount again.
Leaving half for loaded libraries, etc is quite conservative
if max_map_count is the small-ish 64k default.
That reservation could perhaps a fixed number like 5,000 ?
> > > + */
> > > +return max_map_count / 4;
> >
> > That's 256,000 coroutines, which still sounds incredibly large
> > to me.
>
> Any ideas for tweaking this heuristic?
The awkward thing about this limit is that its hardcoded, and
since it is indeed a "heuristic", we know it is going to be
sub-optimal for some use cases / scenarios.
The worst case upper limit is
num virtio-blk * num threads * num queues
Reducing the number of devices isn't practical if the guest
genuinely needs that many volumes.
Reducing the threads or queues artificially limits the peak
performance of a single disk handling in isolation, while
other disks are idle, so that's not desirable.
So there's no way to cap the worst case scenario, while
still maximising the single disk performance possibilities.
With large VMs with many CPUs and many disks, it could be
reasonable to not expect a real guest to need to maximise
I/O on every disk at the same time, and thus want to put
some cap there to control worst case resource usage.
It feels like it leans towards being able to control the
coroutine pool limit explicitly, as a CLI option, to override
this default hueristic.
> > > +}
> > > +#endif
> > > +
> > > +return UINT_MAX;
> >
> > Why UINT_MAX as a default ? If we can't read procfs, we should
> > assume some much smaller sane default IMHO, that corresponds to
> > what current linux default max_map_count would be.
>
> This line is not Linux-specific. I don't know if other OSes have an
> equivalent to max_map_count.
>
> I agree with defaulting to 64k-ish on Linux.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 08/22] plugins: Use emit_before_op for PLUGIN_GEN_FROM_TB
On 3/19/24 03:22, Pierrick Bouvier wrote:
@@ -798,6 +816,25 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
assert(insn != NULL);
gen_disable_mem_helper(plugin_tb, insn);
break;
+
+ case PLUGIN_GEN_FROM_TB:
+ assert(insn == NULL);
+
+ cbs = plugin_tb->cbs[PLUGIN_CB_REGULAR];
+ for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
+ struct qemu_plugin_dyn_cb *cb =
+ _array_index(cbs, struct qemu_plugin_dyn_cb, i);
+ gen_udata_cb(cb);
+ }
+
+ cbs = plugin_tb->cbs[PLUGIN_CB_INLINE];
+ for (i = 0, n = (cbs ? cbs->len : 0); i < n; i++) {
+ struct qemu_plugin_dyn_cb *cb =
+ _array_index(cbs, struct qemu_plugin_dyn_cb, i);
+ gen_inline_cb(cb);
+ }
+ break;
+
Maybe I am missing something, but couldn't we simply mix all cbs possible. This way, the
order mentioned by user when registering is the only one that matters, and he can select
to mix callbacks and inline ops freely.
Just checking the type of callback would be needed to know which gen_* fn
should be used.
See patch 15. :-)
r~
Re: [PATCH 07/22] plugins: Use emit_before_op for PLUGIN_GEN_AFTER_INSN
On 3/19/24 03:32, Pierrick Bouvier wrote:
static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
{
- TCGOp *op;
+ TCGOp *op, *next;
int insn_idx = -1;
pr_ops();
- QTAILQ_FOREACH(op, _ctx->ops, link) {
+ /*
+ * While injecting code, we cannot afford to reuse any ebb temps
+ * that might be live within the existing opcode stream.
+ * The simplest solution is to release them all and create new.
+ */
+ memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps));
+
Not an expert at this, but wouldn't that break an existing TB that already has some ops on
those temps?
No, this only affects allocation of new temps -- if free_temps is empty, a new temp will
be allocated from tcg_ctx->nb_temps++.
Zeroing free_temps here ensures that we *do not* reuse a temp that might already be live
across any plugin insertion point. Between insertion points, we will free plugin temps
and only reuse those.
r~
Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs
On Tue, Mar 19, 2024 at 03:25:18PM -0400, Peter Xu wrote:
> On Tue, Mar 19, 2024 at 04:25:32PM +, Daniel P. Berrangé wrote:
> > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote:
> > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote:
> > > > Peter Xu writes:
> > > >
> > > > > [I queued patch 1-2 into -stable, leaving this patch for further
> > > > > discussions]
> > > > >
> > > > > On Fri, Mar 15, 2024 at 08:55:42AM +, Daniel P. Berrangé wrote:
> > > > >> The 'file:' protocol eventually calls into qemu_open, and this
> > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax
> > > > >> to pass in FDs.
> > > > >
> > > > > If it always use /dev/fdsets for files, does it mean that the newly
> > > > > added
> > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so
> > > > > we can
> > > > > drop them)?
> > > >
> > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the
> > > > MigrationAddress was added. So this:
> > > >
> > > > 'channels': [ { 'channel-type': 'main',
> > > > 'addr': { 'transport': 'socket',
> > > > 'type': 'fd',
> > > > 'str': 'fdname' } } ]
> > > >
> > > > works without multifd and without mapped-ram if the fd is a file or
> > > > socket.
> > > >
> > > > So yes, you're correct, but given we already have this^ it would be
> > > > perhaps more confusing for users to allow it, but not allow the very
> > > > same JSON when multifd=true, mapped-ram=true.
> > >
> > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON
> > > format) is trivial to use. If libvirt didn't use it I won't be surprised
> > > to
> > > see nobody using it. I want us to avoid working on things that nobody is
> > > using, or has a better replacement.
> > >
> > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works
> > > for all the cases that libvirt needs. I am aware that the old getfd has
> > > the monitor limitation so that if the QMP disconnected and reconnect, the
> > > fd can be gone. However I'm not sure whether that's the only reason to
> > > have add-fd, and also not sure whether it means add-fd is always
> > > preferred,
> > > so that maybe we can consider obsolete getfd?
> >
> > Historically libvirt primariily uses the 'fd:' protocol, with a
> > socket FD. It never gives QEMU a plain file FD, since it has
> > always added its "iohelper" as a MITM, in order to add O_DIRECT
> > on top.
> >
> > The 'getfd' command is something that is needed when talking to
> > QEMU for any API that involves a "SocketAddress" QAPI type,
> > which is applicable for migration.
> >
> > With the introduction of 'MigrationAddress', the 'socket' protocol
> > is backed by 'SocketAddress' and thus supports FD passing for
> > sockets (or potentally pipes too), in combination with 'getfd'.
> >
> > With the 'file' protocol in 'MigrationAddress', since it gets
> > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide
> > passing for plain files.
>
> I see. I assume it means we still have multiple users of getfd so it's
> still in use where add-fd is not yet avaiable.
>
> But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole
> mapped-ram effort, neither do we need any support on file migrations over
> "fd", e.g. fd_start_incoming_migration() for files. So we can drop these
> parts, am I right?
Correct, libvirt hasn't got any impl for 'mapped-ram' yet, at least
not something merged.
Since this is new functionality, libvirt could go straight for the
'file' protocol / address type.
At some point I think we can stop using 'fd' for traditional migration
too and pass the socket address to QEMU and let QEMU open the socket.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
