Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
On Wed, Mar 6, 2024 at 9:35 PM Cédric Le Goater wrote:
> Now that the log_global*() handlers take an Error** parameter and
> return a bool, do the same for memory_global_dirty_log_start() and
> memory_global_dirty_log_stop(). The error is reported in the callers
> for now and it will be propagated in the call stack in the next
> changes.
> To be noted a functional change in ram_init_bitmaps(), if the dirty
>
Hi, Cédric Le Goater. Could the functional modification be made
separately from the patch? And my "Reviewed-by" is attached
to the first patch that refines memory_global_dirty_log_start's
function declaration.
> pages logger fails to start, there is no need to synchronize the dirty
> pages bitmaps. colo_incoming_start_dirty_log() could be modified in a
> similar way.
>
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Paul Durrant
> Cc: "Michael S. Tsirkin"
> Cc: Paolo Bonzini
> Cc: David Hildenbrand
> Cc: Hyman Huang
> Reviewed-by: Hyman Huang
> Signed-off-by: Cédric Le Goater
> ---
>
> Changes in v4:
>
> - Dropped log_global_stop() and log_global_sync() changes
>
> include/exec/memory.h | 5 -
> hw/i386/xen/xen-hvm.c | 2 +-
> migration/dirtyrate.c | 13 +++--
> migration/ram.c | 22 --
> system/memory.c | 11 +--
> 5 files changed, 41 insertions(+), 12 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index
> 567bc4c9fdb53e8f63487f1400980275687d..c129ee6db7162504bd72d4cfc69b5affb2cd87e8
> 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2570,8 +2570,11 @@ void memory_listener_unregister(MemoryListener
> *listener);
> * memory_global_dirty_log_start: begin dirty logging for all regions
> *
> * @flags: purpose of starting dirty log, migration or dirty rate
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Return: true on success, else false setting @errp with error.
> */
> -void memory_global_dirty_log_start(unsigned int flags);
> +bool memory_global_dirty_log_start(unsigned int flags, Error **errp);
>
> /**
> * memory_global_dirty_log_stop: end dirty logging for all regions
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index
> 0608ca99f5166fd6379ee674442484e805eff9c0..57cb7df50788a6c31eff68c95e8eaa856fdebede
> 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -654,7 +654,7 @@ void xen_hvm_modified_memory(ram_addr_t start,
> ram_addr_t length)
> void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
> {
> if (enable) {
> -memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
> +memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
> } else {
> memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
> }
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index
> 1d2e85746fb7b10eb7f149976970f9a92125af8a..d02d70b7b4b86a29d4d5540ded416543536d8f98
> 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -90,9 +90,15 @@ static int64_t do_calculate_dirtyrate(DirtyPageRecord
> dirty_pages,
>
> void global_dirty_log_change(unsigned int flag, bool start)
> {
> +Error *local_err = NULL;
> +bool ret;
> +
> bql_lock();
> if (start) {
> -memory_global_dirty_log_start(flag);
> +ret = memory_global_dirty_log_start(flag, _err);
> +if (!ret) {
> +error_report_err(local_err);
> +}
> } else {
> memory_global_dirty_log_stop(flag);
> }
> @@ -608,9 +614,12 @@ static void calculate_dirtyrate_dirty_bitmap(struct
> DirtyRateConfig config)
> {
> int64_t start_time;
> DirtyPageRecord dirty_pages;
> +Error *local_err = NULL;
>
> bql_lock();
> -memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE);
> +if (!memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE,
> _err)) {
> +error_report_err(local_err);
> +}
>
> /*
> * 1'round of log sync may return all 1 bits with
> diff --git a/migration/ram.c b/migration/ram.c
> index
> c5149b7d717aefad7f590422af0ea4a40e7507be..397b4c0f218a66d194e44f9c5f9fe8e9885c48b6
> 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2836,18 +2836,31 @@ static void
> migration_bitmap_clear_discarded_pages(RAMState *rs)
>
> static void ram_init_bitmaps(RAMState *rs)
> {
> +Error *local_err = NULL;
> +bool ret = true;
> +
> qemu_mutex_lock_ramlist();
>
> WITH_RCU_READ_LOCK_GUARD() {
> ram_list_init_bitmaps();
> /* We don't use dirty log with background snapshots */
> if (!migrate_background_snapshot()) {
> -memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
> +ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION,
> +_err);
> +if (!ret) {
> +error_report_err(local_err);
> +goto out_unlock;
> +}
>
[PATCH 20/22] plugins: Move qemu_plugin_insn_cleanup_fn to tcg.c
This is only used in one place, and usage requires an
out-of-line function.
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 12
tcg/tcg.c | 12
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 07b1755990..201889cbee 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -116,18 +116,6 @@ struct qemu_plugin_scoreboard {
QLIST_ENTRY(qemu_plugin_scoreboard) entry;
};
-/*
- * qemu_plugin_insn allocate and cleanup functions. We don't expect to
- * cleanup many of these structures. They are reused for each fresh
- * translation.
- */
-
-static inline void qemu_plugin_insn_cleanup_fn(gpointer data)
-{
-struct qemu_plugin_insn *insn = (struct qemu_plugin_insn *) data;
-g_byte_array_free(insn->data, true);
-}
-
/* Internal context for this TranslationBlock */
struct qemu_plugin_tb {
GPtrArray *insns;
diff --git a/tcg/tcg.c b/tcg/tcg.c
index d248c52e96..d7abc514c4 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -761,6 +761,18 @@ QEMU_BUILD_BUG_ON((int)(offsetof(CPUNegativeOffsetState,
tlb.f[0]) -
< MIN_TLB_MASK_TABLE_OFS);
#endif
+#ifdef CONFIG_PLUGIN
+/*
+ * We don't expect to cleanup many of these structures.
+ * They are reused for each fresh translation.
+ */
+static void qemu_plugin_insn_cleanup_fn(gpointer data)
+{
+struct qemu_plugin_insn *insn = (struct qemu_plugin_insn *) data;
+g_byte_array_free(insn->data, true);
+}
+#endif
+
static void alloc_tcg_plugin_context(TCGContext *s)
{
#ifdef CONFIG_PLUGIN
--
2.34.1
[PATCH 06/22] plugins: Create TCGHelperInfo for all out-of-line callbacks
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 *)
+ */
+[0 ... 2].typemask = (dh_typemask(void, 0) |
+ dh_typemask(i32, 1) |
+ dh_typemask(ptr, 2))
+};
+struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr);
dyn_cb->userp = udata;
-/* Note flags are discarded as unused. */
-dyn_cb->regular.f.vcpu_udata = cb;
dyn_cb->type = PLUGIN_CB_REGULAR;
+dyn_cb->regular.f.vcpu_udata = cb;
+
+assert((unsigned)flags < ARRAY_SIZE(info));
+dyn_cb->regular.info = [flags];
}
void plugin_register_vcpu_mem_cb(GArray **arr,
@@ -352,14 +366,39 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
enum qemu_plugin_mem_rw rw,
void *udata)
{
-struct qemu_plugin_dyn_cb *dyn_cb;
+/*
+ * Expect that the underlying type for enum qemu_plugin_meminfo_t
+ * is either int32_t or uint32_t, aka int or unsigned int.
+ */
+QEMU_BUILD_BUG_ON(
+!__builtin_types_compatible_p(qemu_plugin_meminfo_t, uint32_t) &&
+!__builtin_types_compatible_p(qemu_plugin_meminfo_t, int32_t));
-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_mem_cb_t:
+ * void (*)(uint32_t, qemu_plugin_meminfo_t, uint64_t, void *)
+ */
+[0 ... 2].typemask =
+(dh_typemask(void, 0) |
+ dh_typemask(i32, 1) |
+ (__builtin_types_compatible_p(qemu_plugin_meminfo_t, uint32_t)
+ ? dh_typemask(i32, 2) : dh_typemask(s32, 2)) |
+ dh_typemask(i64, 3) |
+ dh_typemask(ptr, 4))
+};
+
+struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr);
dyn_cb->userp = udata;
-/* Note flags are discarded as unused. */
dyn_cb->type = PLUGIN_CB_REGULAR;
dyn_cb->rw = rw;
dyn_cb->regular.f.vcpu_mem = cb;
+
+assert((unsigned)flags < ARRAY_SIZE(info));
+dyn_cb->regular.info = [flags];
}
/*
--
2.34.1
[PATCH 02/22] tcg: Make tcg/helper-info.h self-contained
Move MAX_CALL_IARGS from tcg.h and include for the define of TCG_TARGET_REG_BITS. Signed-off-by: Richard Henderson --- include/tcg/helper-info.h | 3 +++ include/tcg/tcg.h | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/tcg/helper-info.h b/include/tcg/helper-info.h index 7c27d6164a..909fe73afa 100644 --- a/include/tcg/helper-info.h +++ b/include/tcg/helper-info.h @@ -12,6 +12,9 @@ #ifdef CONFIG_TCG_INTERPRETER #include #endif +#include "tcg-target-reg-bits.h" + +#define MAX_CALL_IARGS 7 /* * Describe the calling convention of a given argument type. diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h index e9d05f40b0..a6e7df146a 100644 --- a/include/tcg/tcg.h +++ b/include/tcg/tcg.h @@ -39,8 +39,6 @@ /* XXX: make safe guess about sizes */ #define MAX_OP_PER_INSTR 266 -#define MAX_CALL_IARGS 7 - #define CPU_TEMP_BUF_NLONGS 128 #define TCG_STATIC_FRAME_SIZE (CPU_TEMP_BUF_NLONGS * sizeof(long)) -- 2.34.1
[PATCH 05/22] plugins: Move function pointer in qemu_plugin_dyn_cb
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;
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 8028786c7b..c56f104aee 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -431,7 +431,7 @@ static TCGOp *append_udata_cb(const struct
qemu_plugin_dyn_cb *cb,
}
/* call */
-op = copy_call(_op, op, cb->f.vcpu_udata, cb_idx);
+op = copy_call(_op, op, cb->regular.f.vcpu_udata, cb_idx);
return op;
}
@@ -479,7 +479,7 @@ static TCGOp *append_mem_cb(const struct qemu_plugin_dyn_cb
*cb,
if (type == PLUGIN_GEN_CB_MEM) {
/* call */
-op = copy_call(_op, op, cb->f.vcpu_udata, cb_idx);
+op = copy_call(_op, op, cb->regular.f.vcpu_udata, cb_idx);
}
return op;
diff --git a/plugins/core.c b/plugins/core.c
index 4487cb7c48..837c373690 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -342,7 +342,7 @@ void plugin_register_dyn_cb__udata(GArray **arr,
dyn_cb->userp = udata;
/* Note flags are discarded as unused. */
-dyn_cb->f.vcpu_udata = cb;
+dyn_cb->regular.f.vcpu_udata = cb;
dyn_cb->type = PLUGIN_CB_REGULAR;
}
@@ -359,7 +359,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
/* Note flags are discarded as unused. */
dyn_cb->type = PLUGIN_CB_REGULAR;
dyn_cb->rw = rw;
-dyn_cb->f.generic = cb;
+dyn_cb->regular.f.vcpu_mem = cb;
}
/*
@@ -511,8 +511,8 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
}
switch (cb->type) {
case PLUGIN_CB_REGULAR:
-cb->f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw),
- vaddr, cb->userp);
+cb->regular.f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw),
+ vaddr, cb->userp);
break;
case PLUGIN_CB_INLINE:
exec_inline_op(cb, cpu->cpu_index);
--
2.34.1
[PATCH 17/22] plugins: Replace pr_ops with a proper debug dump flag
The DEBUG_PLUGIN_GEN_OPS ifdef is replaced with "-d op_plugin".
The second pr_ops call can be obtained with "-d op".
Signed-off-by: Richard Henderson
---
include/qemu/log.h | 1 +
include/tcg/tcg.h | 1 +
accel/tcg/plugin-gen.c | 68 --
tcg/tcg.c | 29 +-
util/log.c | 4 +++
5 files changed, 46 insertions(+), 57 deletions(-)
diff --git a/include/qemu/log.h b/include/qemu/log.h
index df59bfabcd..e10e24cd4f 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -36,6 +36,7 @@ bool qemu_log_separate(void);
#define LOG_STRACE (1 << 19)
#define LOG_PER_THREAD (1 << 20)
#define CPU_LOG_TB_VPU (1 << 21)
+#define LOG_TB_OP_PLUGIN (1 << 22)
/* Lock/unlock output. */
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index df66e8f012..753d7ca3e0 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -1065,5 +1065,6 @@ static inline const TCGOpcode *tcg_swap_vecop_list(const
TCGOpcode *n)
}
bool tcg_can_emit_vecop_list(const TCGOpcode *, TCGType, unsigned);
+void tcg_dump_ops(TCGContext *s, FILE *f, bool have_prefs);
#endif /* TCG_H */
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 6f0731b479..10d917abd3 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -44,6 +44,7 @@
*/
#include "qemu/osdep.h"
#include "qemu/plugin.h"
+#include "qemu/log.h"
#include "cpu.h"
#include "tcg/tcg.h"
#include "tcg/tcg-temp-internal.h"
@@ -58,6 +59,7 @@
# define CONFIG_SOFTMMU_GATE 0
#endif
+/* Update plugin_from_name in tcg.c. */
enum plugin_gen_from {
PLUGIN_GEN_FROM_TB,
PLUGIN_GEN_FROM_INSN,
@@ -192,66 +194,21 @@ static void gen_mem_cb(struct qemu_plugin_dyn_cb *cb,
tcg_temp_free_i32(cpu_index);
}
-/* #define DEBUG_PLUGIN_GEN_OPS */
-static void pr_ops(void)
-{
-#ifdef DEBUG_PLUGIN_GEN_OPS
-TCGOp *op;
-int i = 0;
-
-QTAILQ_FOREACH(op, _ctx->ops, link) {
-const char *name = "";
-const char *type = "";
-
-if (op->opc == INDEX_op_plugin_cb_start) {
-switch (op->args[0]) {
-case PLUGIN_GEN_FROM_TB:
-name = "tb";
-break;
-case PLUGIN_GEN_FROM_INSN:
-name = "insn";
-break;
-case PLUGIN_GEN_FROM_MEM:
-name = "mem";
-break;
-case PLUGIN_GEN_AFTER_INSN:
-name = "after insn";
-break;
-default:
-break;
-}
-switch (op->args[1]) {
-case PLUGIN_GEN_CB_UDATA:
-type = "udata";
-break;
-case PLUGIN_GEN_CB_INLINE:
-type = "inline";
-break;
-case PLUGIN_GEN_CB_MEM:
-type = "mem";
-break;
-case PLUGIN_GEN_ENABLE_MEM_HELPER:
-type = "enable mem helper";
-break;
-case PLUGIN_GEN_DISABLE_MEM_HELPER:
-type = "disable mem helper";
-break;
-default:
-break;
-}
-}
-printf("op[%2i]: %s %s %s\n", i, tcg_op_defs[op->opc].name, name,
type);
-i++;
-}
-#endif
-}
-
static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
{
TCGOp *op, *next;
int insn_idx = -1;
-pr_ops();
+if (unlikely(qemu_loglevel_mask(LOG_TB_OP_PLUGIN)
+ && qemu_log_in_addr_range(plugin_tb->vaddr))) {
+FILE *logfile = qemu_log_trylock();
+if (logfile) {
+fprintf(logfile, "OP before plugin injection:\n");
+tcg_dump_ops(tcg_ctx, logfile, false);
+fprintf(logfile, "\n");
+qemu_log_unlock(logfile);
+}
+}
/*
* While injecting code, we cannot afford to reuse any ebb temps
@@ -389,7 +346,6 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
break;
}
}
-pr_ops();
}
bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 363a065e28..d248c52e96 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2540,6 +2540,15 @@ static const char bswap_flag_name[][6] = {
[TCG_BSWAP_IZ | TCG_BSWAP_OS] = "iz,os",
};
+#ifdef CONFIG_PLUGIN
+static const char * const plugin_from_name[] = {
+"from-tb",
+"from-insn",
+"after-insn",
+"after-tb",
+};
+#endif
+
static inline bool tcg_regset_single(TCGRegSet d)
{
return (d & (d - 1)) == 0;
@@ -2558,7 +2567,7 @@ static inline TCGReg tcg_regset_first(TCGRegSet d)
#define ne_fprintf(...) \
({ int ret_ = fprintf(__VA_ARGS__); ret_ >= 0 ? ret_ : 0; })
-static void tcg_dump_ops(TCGContext *s, FILE *f, bool have_prefs)
+void tcg_dump_ops(TCGContext *s, FILE *f, bool have_prefs)
{
char buf[128];
TCGOp *op;
@@ -2714,6 +2723,24 @@ static void tcg_dump_ops(TCGContext
[PATCH 19/22] plugins: Merge qemu_plugin_tb_insn_get to plugin-gen.c
Merge qemu_plugin_insn_alloc and qemu_plugin_tb_insn_get into
plugin_gen_insn_start, since it is used nowhere else.
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 39 ---
accel/tcg/plugin-gen.c | 39 ---
2 files changed, 32 insertions(+), 46 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 34498da717..07b1755990 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -128,14 +128,6 @@ static inline void qemu_plugin_insn_cleanup_fn(gpointer
data)
g_byte_array_free(insn->data, true);
}
-static inline struct qemu_plugin_insn *qemu_plugin_insn_alloc(void)
-{
-struct qemu_plugin_insn *insn = g_new0(struct qemu_plugin_insn, 1);
-
-insn->data = g_byte_array_sized_new(4);
-return insn;
-}
-
/* Internal context for this TranslationBlock */
struct qemu_plugin_tb {
GPtrArray *insns;
@@ -152,37 +144,6 @@ struct qemu_plugin_tb {
GArray *cbs;
};
-/**
- * qemu_plugin_tb_insn_get(): get next plugin record for translation.
- * @tb: the internal tb context
- * @pc: address of instruction
- */
-static inline
-struct qemu_plugin_insn *qemu_plugin_tb_insn_get(struct qemu_plugin_tb *tb,
- uint64_t pc)
-{
-struct qemu_plugin_insn *insn;
-
-if (unlikely(tb->n == tb->insns->len)) {
-struct qemu_plugin_insn *new_insn = qemu_plugin_insn_alloc();
-g_ptr_array_add(tb->insns, new_insn);
-}
-
-insn = g_ptr_array_index(tb->insns, tb->n++);
-g_byte_array_set_size(insn->data, 0);
-insn->calls_helpers = false;
-insn->mem_helper = false;
-insn->vaddr = pc;
-if (insn->insn_cbs) {
-g_array_set_size(insn->insn_cbs, 0);
-}
-if (insn->mem_cbs) {
-g_array_set_size(insn->mem_cbs, 0);
-}
-
-return insn;
-}
-
/**
* struct CPUPluginState - per-CPU state for plugins
* @event_mask: plugin event bitmap. Modified only via async work.
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 28414c4ff1..70914c3bf8 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -380,11 +380,34 @@ bool plugin_gen_tb_start(CPUState *cpu, const
DisasContextBase *db,
void plugin_gen_insn_start(CPUState *cpu, const DisasContextBase *db)
{
struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;
-struct qemu_plugin_insn *pinsn;
+struct qemu_plugin_insn *insn;
+size_t n = db->num_insns;
+vaddr pc;
-pinsn = qemu_plugin_tb_insn_get(ptb, db->pc_next);
-tcg_ctx->plugin_insn = pinsn;
-plugin_gen_empty_callback(PLUGIN_GEN_FROM_INSN);
+assert(n >= 1);
+ptb->n = n;
+if (n <= ptb->insns->len) {
+insn = g_ptr_array_index(ptb->insns, n - 1);
+g_byte_array_set_size(insn->data, 0);
+} else {
+assert(n - 1 == ptb->insns->len);
+insn = g_new0(struct qemu_plugin_insn, 1);
+insn->data = g_byte_array_sized_new(4);
+g_ptr_array_add(ptb->insns, insn);
+}
+
+tcg_ctx->plugin_insn = insn;
+insn->calls_helpers = false;
+insn->mem_helper = false;
+if (insn->insn_cbs) {
+g_array_set_size(insn->insn_cbs, 0);
+}
+if (insn->mem_cbs) {
+g_array_set_size(insn->mem_cbs, 0);
+}
+
+pc = db->pc_next;
+insn->vaddr = pc;
/*
* Detect page crossing to get the new host address.
@@ -392,16 +415,18 @@ void plugin_gen_insn_start(CPUState *cpu, const
DisasContextBase *db)
* fetching instructions from a region not backed by RAM.
*/
if (ptb->haddr1 == NULL) {
-pinsn->haddr = NULL;
+insn->haddr = NULL;
} else if (is_same_page(db, db->pc_next)) {
-pinsn->haddr = ptb->haddr1 + pinsn->vaddr - ptb->vaddr;
+insn->haddr = ptb->haddr1 + pc - ptb->vaddr;
} else {
if (ptb->vaddr2 == -1) {
ptb->vaddr2 = TARGET_PAGE_ALIGN(db->pc_first);
get_page_addr_code_hostp(cpu_env(cpu), ptb->vaddr2, >haddr2);
}
-pinsn->haddr = ptb->haddr2 + pinsn->vaddr - ptb->vaddr2;
+insn->haddr = ptb->haddr2 + pc - ptb->vaddr2;
}
+
+plugin_gen_empty_callback(PLUGIN_GEN_FROM_INSN);
}
void plugin_gen_insn_end(void)
--
2.34.1
[PATCH 04/22] plugins: Zero new qemu_plugin_dyn_cb entries
Signed-off-by: Richard Henderson
---
plugins/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plugins/core.c b/plugins/core.c
index 11ca20e626..4487cb7c48 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -307,7 +307,7 @@ static struct qemu_plugin_dyn_cb *plugin_get_dyn_cb(GArray
**arr)
GArray *cbs = *arr;
if (!cbs) {
-cbs = g_array_sized_new(false, false,
+cbs = g_array_sized_new(false, true,
sizeof(struct qemu_plugin_dyn_cb), 1);
*arr = cbs;
}
--
2.34.1
[PATCH 21/22] plugins: Inline plugin_gen_empty_callback
Each caller can use tcg_gen_plugin_cb directly.
Signed-off-by: Richard Henderson
---
accel/tcg/plugin-gen.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 70914c3bf8..fd52ea3987 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -67,19 +67,6 @@ enum plugin_gen_from {
PLUGIN_GEN_AFTER_TB,
};
-static void plugin_gen_empty_callback(enum plugin_gen_from from)
-{
-switch (from) {
-case PLUGIN_GEN_AFTER_INSN:
-case PLUGIN_GEN_FROM_TB:
-case PLUGIN_GEN_FROM_INSN:
-tcg_gen_plugin_cb(from);
-break;
-default:
-g_assert_not_reached();
-}
-}
-
/* called before finishing a TB with exit_tb, goto_tb or goto_ptr */
void plugin_gen_disable_mem_helpers(void)
{
@@ -369,7 +356,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const
DisasContextBase *db,
ptb->mem_only = mem_only;
ptb->mem_helper = false;
-plugin_gen_empty_callback(PLUGIN_GEN_FROM_TB);
+tcg_gen_plugin_cb(PLUGIN_GEN_FROM_TB);
}
tcg_ctx->plugin_insn = NULL;
@@ -426,12 +413,12 @@ void plugin_gen_insn_start(CPUState *cpu, const
DisasContextBase *db)
insn->haddr = ptb->haddr2 + pc - ptb->vaddr2;
}
-plugin_gen_empty_callback(PLUGIN_GEN_FROM_INSN);
+tcg_gen_plugin_cb(PLUGIN_GEN_FROM_INSN);
}
void plugin_gen_insn_end(void)
{
-plugin_gen_empty_callback(PLUGIN_GEN_AFTER_INSN);
+tcg_gen_plugin_cb(PLUGIN_GEN_AFTER_INSN);
}
/*
--
2.34.1
[PATCH 16/22] plugins: Introduce PLUGIN_CB_MEM_REGULAR
Use different enumerators for vcpu_udata and vcpu_mem callbacks.
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 1 +
accel/tcg/plugin-gen.c | 2 +-
plugins/core.c | 4 ++--
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index cf9758be55..34498da717 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -67,6 +67,7 @@ union qemu_plugin_cb_sig {
enum plugin_dyn_cb_type {
PLUGIN_CB_REGULAR,
+PLUGIN_CB_MEM_REGULAR,
PLUGIN_CB_INLINE,
};
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 669e343cfb..6f0731b479 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -367,7 +367,7 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
if (cb->rw & rw) {
switch (cb->type) {
-case PLUGIN_CB_REGULAR:
+case PLUGIN_CB_MEM_REGULAR:
gen_mem_cb(cb, meminfo, addr);
break;
case PLUGIN_CB_INLINE:
diff --git a/plugins/core.c b/plugins/core.c
index b0615f1e7f..0213513ec6 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -391,7 +391,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr);
dyn_cb->userp = udata;
-dyn_cb->type = PLUGIN_CB_REGULAR;
+dyn_cb->type = PLUGIN_CB_MEM_REGULAR;
dyn_cb->rw = rw;
dyn_cb->regular.f.vcpu_mem = cb;
@@ -547,7 +547,7 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
break;
}
switch (cb->type) {
-case PLUGIN_CB_REGULAR:
+case PLUGIN_CB_MEM_REGULAR:
cb->regular.f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw),
vaddr, cb->userp);
break;
--
2.34.1
[PATCH 18/22] plugins: Split out common cb expanders
Signed-off-by: Richard Henderson
---
accel/tcg/plugin-gen.c | 84 +-
1 file changed, 41 insertions(+), 43 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 10d917abd3..28414c4ff1 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -194,6 +194,37 @@ static void gen_mem_cb(struct qemu_plugin_dyn_cb *cb,
tcg_temp_free_i32(cpu_index);
}
+static void inject_cb(struct qemu_plugin_dyn_cb *cb)
+
+{
+switch (cb->type) {
+case PLUGIN_CB_REGULAR:
+gen_udata_cb(cb);
+break;
+case PLUGIN_CB_INLINE:
+gen_inline_cb(cb);
+break;
+default:
+g_assert_not_reached();
+}
+}
+
+static void inject_mem_cb(struct qemu_plugin_dyn_cb *cb,
+ enum qemu_plugin_mem_rw rw,
+ qemu_plugin_meminfo_t meminfo, TCGv_i64 addr)
+{
+if (cb->rw & rw) {
+switch (cb->type) {
+case PLUGIN_CB_MEM_REGULAR:
+gen_mem_cb(cb, meminfo, addr);
+break;
+default:
+inject_cb(cb);
+break;
+}
+}
+}
+
static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
{
TCGOp *op, *next;
@@ -255,19 +286,8 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
cbs = plugin_tb->cbs;
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);
-
-switch (cb->type) {
-case PLUGIN_CB_REGULAR:
-gen_udata_cb(cb);
-break;
-case PLUGIN_CB_INLINE:
-gen_inline_cb(cb);
-break;
-default:
-g_assert_not_reached();
-}
+inject_cb(
+_array_index(cbs, struct qemu_plugin_dyn_cb, i));
}
break;
@@ -278,19 +298,8 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
cbs = insn->insn_cbs;
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);
-
-switch (cb->type) {
-case PLUGIN_CB_REGULAR:
-gen_udata_cb(cb);
-break;
-case PLUGIN_CB_INLINE:
-gen_inline_cb(cb);
-break;
-default:
-g_assert_not_reached();
-}
+inject_cb(
+_array_index(cbs, struct qemu_plugin_dyn_cb, i));
}
break;
@@ -307,33 +316,22 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
{
TCGv_i64 addr = temp_tcgv_i64(arg_temp(op->args[0]));
qemu_plugin_meminfo_t meminfo = op->args[1];
+enum qemu_plugin_mem_rw rw =
+(qemu_plugin_mem_is_store(meminfo)
+ ? QEMU_PLUGIN_MEM_W : QEMU_PLUGIN_MEM_R);
struct qemu_plugin_insn *insn;
const GArray *cbs;
-int i, n, rw;
+int i, n;
assert(insn_idx >= 0);
insn = g_ptr_array_index(plugin_tb->insns, insn_idx);
-rw = qemu_plugin_mem_is_store(meminfo) ? 2 : 1;
tcg_ctx->emit_before_op = op;
cbs = insn->mem_cbs;
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);
-
-if (cb->rw & rw) {
-switch (cb->type) {
-case PLUGIN_CB_MEM_REGULAR:
-gen_mem_cb(cb, meminfo, addr);
-break;
-case PLUGIN_CB_INLINE:
-gen_inline_cb(cb);
-break;
-default:
-g_assert_not_reached();
-}
-}
+inject_mem_cb(_array_index(cbs, struct qemu_plugin_dyn_cb,
i),
+ rw, meminfo, addr);
}
tcg_ctx->emit_before_op = NULL;
--
2.34.1
[PATCH 22/22] plugins: Update the documentation block for plugin-gen.c
Signed-off-by: Richard Henderson --- accel/tcg/plugin-gen.c | 31 --- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c index fd52ea3987..c354825779 100644 --- a/accel/tcg/plugin-gen.c +++ b/accel/tcg/plugin-gen.c @@ -14,33 +14,10 @@ * Injecting the desired instrumentation could be done with a second * translation pass that combined the instrumentation requests, but that * would be ugly and inefficient since we would decode the guest code twice. - * Instead, during TB translation we add "empty" instrumentation calls for all - * possible instrumentation events, and then once we collect the instrumentation - * requests from plugins, we either "fill in" those empty events or remove them - * if they have no requests. - * - * When "filling in" an event we first copy the empty callback's TCG ops. This - * might seem unnecessary, but it is done to support an arbitrary number - * of callbacks per event. Take for example a regular instruction callback. - * We first generate a callback to an empty helper function. Then, if two - * plugins register one callback each for this instruction, we make two copies - * of the TCG ops generated for the empty callback, substituting the function - * pointer that points to the empty helper function with the plugins' desired - * callback functions. After that we remove the empty callback's ops. - * - * Note that the location in TCGOp.args[] of the pointer to a helper function - * varies across different guest and host architectures. Instead of duplicating - * the logic that figures this out, we rely on the fact that the empty - * callbacks point to empty functions that are unique pointers in the program. - * Thus, to find the right location we just have to look for a match in - * TCGOp.args[]. This is the main reason why we first copy an empty callback's - * TCG ops and then fill them in; regardless of whether we have one or many - * callbacks for that event, the logic to add all of them is the same. - * - * When generating more than one callback per event, we make a small - * optimization to avoid generating redundant operations. For instance, for the - * second and all subsequent callbacks of an event, we do not need to reload the - * CPU's index into a TCG temp, since the first callback did it already. + * Instead, during TB translation we add "plugin_cb" marker opcodes + * for all possible instrumentation events, and then once we collect the + * instrumentation requests from plugins, we generate code for those markers + * or remove them if they have no requests. */ #include "qemu/osdep.h" #include "qemu/plugin.h" -- 2.34.1
[PATCH 13/22] tcg: Remove TCG_CALL_PLUGIN
Since we no longer emit plugin helpers during the initial code
translation phase, we don't need to specially mark plugin helpers.
Signed-off-by: Richard Henderson
---
include/tcg/tcg.h | 2 --
plugins/core.c| 10 --
tcg/tcg.c | 4 +---
3 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 95a7f4d010..df66e8f012 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -353,8 +353,6 @@ typedef TCGv_ptr TCGv_env;
#define TCG_CALL_NO_SIDE_EFFECTS0x0004
/* Helper is G_NORETURN. */
#define TCG_CALL_NO_RETURN 0x0008
-/* Helper is part of Plugins. */
-#define TCG_CALL_PLUGIN 0x0010
/* convenience version of most used call flags */
#define TCG_CALL_NO_RWG TCG_CALL_NO_READ_GLOBALS
diff --git a/plugins/core.c b/plugins/core.c
index b0a2e80874..b0615f1e7f 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -339,9 +339,8 @@ void plugin_register_dyn_cb__udata(GArray **arr,
void *udata)
{
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,
+[QEMU_PLUGIN_CB_NO_REGS].flags = TCG_CALL_NO_RWG,
+[QEMU_PLUGIN_CB_R_REGS].flags = TCG_CALL_NO_WG,
/*
* Match qemu_plugin_vcpu_udata_cb_t:
* void (*)(uint32_t, void *)
@@ -375,9 +374,8 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
!__builtin_types_compatible_p(qemu_plugin_meminfo_t, int32_t));
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,
+[QEMU_PLUGIN_CB_NO_REGS].flags = TCG_CALL_NO_RWG,
+[QEMU_PLUGIN_CB_R_REGS].flags = TCG_CALL_NO_WG,
/*
* Match qemu_plugin_vcpu_mem_cb_t:
* void (*)(uint32_t, qemu_plugin_meminfo_t, uint64_t, void *)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 0bf218314b..363a065e28 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2269,9 +2269,7 @@ static void tcg_gen_callN(void *func, TCGHelperInfo *info,
#ifdef CONFIG_PLUGIN
/* Flag helpers that may affect guest state */
-if (tcg_ctx->plugin_insn &&
-!(info->flags & TCG_CALL_PLUGIN) &&
-!(info->flags & TCG_CALL_NO_SIDE_EFFECTS)) {
+if (tcg_ctx->plugin_insn && !(info->flags & TCG_CALL_NO_SIDE_EFFECTS)) {
tcg_ctx->plugin_insn->calls_helpers = true;
}
#endif
--
2.34.1
[PATCH 15/22] plugins: Simplify callback queues
We have qemu_plugin_dyn_cb.type to differentiate the various
callback types, so we do not need to keep them in separate queues.
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 35 ++--
accel/tcg/plugin-gen.c | 90 ++
plugins/api.c | 18 +++--
3 files changed, 65 insertions(+), 78 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index ee1c1b174a..cf9758be55 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -66,15 +66,8 @@ union qemu_plugin_cb_sig {
};
enum plugin_dyn_cb_type {
-PLUGIN_CB_INSN,
-PLUGIN_CB_MEM,
-PLUGIN_N_CB_TYPES,
-};
-
-enum plugin_dyn_cb_subtype {
PLUGIN_CB_REGULAR,
PLUGIN_CB_INLINE,
-PLUGIN_N_CB_SUBTYPES,
};
/*
@@ -84,7 +77,7 @@ enum plugin_dyn_cb_subtype {
*/
struct qemu_plugin_dyn_cb {
void *userp;
-enum plugin_dyn_cb_subtype type;
+enum plugin_dyn_cb_type 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 */
@@ -106,7 +99,8 @@ struct qemu_plugin_insn {
GByteArray *data;
uint64_t vaddr;
void *haddr;
-GArray *cbs[PLUGIN_N_CB_TYPES][PLUGIN_N_CB_SUBTYPES];
+GArray *insn_cbs;
+GArray *mem_cbs;
bool calls_helpers;
/* if set, the instruction calls helpers that might access guest memory */
@@ -135,16 +129,9 @@ static inline void qemu_plugin_insn_cleanup_fn(gpointer
data)
static inline struct qemu_plugin_insn *qemu_plugin_insn_alloc(void)
{
-int i, j;
struct qemu_plugin_insn *insn = g_new0(struct qemu_plugin_insn, 1);
-insn->data = g_byte_array_sized_new(4);
-for (i = 0; i < PLUGIN_N_CB_TYPES; i++) {
-for (j = 0; j < PLUGIN_N_CB_SUBTYPES; j++) {
-insn->cbs[i][j] = g_array_new(false, false,
- sizeof(struct qemu_plugin_dyn_cb));
-}
-}
+insn->data = g_byte_array_sized_new(4);
return insn;
}
@@ -161,7 +148,7 @@ struct qemu_plugin_tb {
/* if set, the TB calls helpers that might access guest memory */
bool mem_helper;
-GArray *cbs[PLUGIN_N_CB_SUBTYPES];
+GArray *cbs;
};
/**
@@ -174,22 +161,22 @@ struct qemu_plugin_insn *qemu_plugin_tb_insn_get(struct
qemu_plugin_tb *tb,
uint64_t pc)
{
struct qemu_plugin_insn *insn;
-int i, j;
if (unlikely(tb->n == tb->insns->len)) {
struct qemu_plugin_insn *new_insn = qemu_plugin_insn_alloc();
g_ptr_array_add(tb->insns, new_insn);
}
+
insn = g_ptr_array_index(tb->insns, tb->n++);
g_byte_array_set_size(insn->data, 0);
insn->calls_helpers = false;
insn->mem_helper = false;
insn->vaddr = pc;
-
-for (i = 0; i < PLUGIN_N_CB_TYPES; i++) {
-for (j = 0; j < PLUGIN_N_CB_SUBTYPES; j++) {
-g_array_set_size(insn->cbs[i][j], 0);
-}
+if (insn->insn_cbs) {
+g_array_set_size(insn->insn_cbs, 0);
+}
+if (insn->mem_cbs) {
+g_array_set_size(insn->mem_cbs, 0);
}
return insn;
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index c8f0e0ecaa..669e343cfb 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -89,9 +89,8 @@ void plugin_gen_disable_mem_helpers(void)
static void gen_enable_mem_helper(struct qemu_plugin_tb *ptb,
struct qemu_plugin_insn *insn)
{
-GArray *cbs[2];
GArray *arr;
-size_t n_cbs;
+size_t len;
/*
* Tracking memory accesses performed from helpers requires extra work.
@@ -110,22 +109,25 @@ static void gen_enable_mem_helper(struct qemu_plugin_tb
*ptb,
return;
}
-cbs[0] = insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_REGULAR];
-cbs[1] = insn->cbs[PLUGIN_CB_MEM][PLUGIN_CB_INLINE];
-n_cbs = cbs[0]->len + cbs[1]->len;
-
-if (n_cbs == 0) {
+if (!insn->mem_cbs || !insn->mem_cbs->len) {
insn->mem_helper = false;
return;
}
insn->mem_helper = true;
ptb->mem_helper = true;
+/*
+ * TODO: It seems like we should be able to use ref/unref
+ * to avoid needing to actually copy this array.
+ * Alternately, perhaps we could allocate new memory adjacent
+ * to the TranslationBlock itself, so that we do not have to
+ * actively manage the lifetime after this.
+ */
+len = insn->mem_cbs->len;
arr = g_array_sized_new(false, false,
-sizeof(struct qemu_plugin_dyn_cb), n_cbs);
-g_array_append_vals(arr, cbs[0]->data, cbs[0]->len);
-g_array_append_vals(arr, cbs[1]->data, cbs[1]->len);
-
+sizeof(struct qemu_plugin_dyn_cb), len);
+memcpy(arr->data, insn->mem_cbs->data,
+ len * sizeof(struct qemu_plugin_dyn_cb));
qemu_plugin_add_dyn_cb_arr(arr);
tcg_gen_st_ptr(tcg_constant_ptr((intptr_t)arr),
[PATCH 10/22] plugins: Use emit_before_op for PLUGIN_GEN_FROM_INSN
Signed-off-by: Richard Henderson
---
include/qemu/plugin.h | 1 -
accel/tcg/plugin-gen.c | 286 ++---
plugins/api.c | 8 +-
3 files changed, 67 insertions(+), 228 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 793c44f1f2..ee1c1b174a 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -73,7 +73,6 @@ enum plugin_dyn_cb_type {
enum plugin_dyn_cb_subtype {
PLUGIN_CB_REGULAR,
-PLUGIN_CB_REGULAR_R,
PLUGIN_CB_INLINE,
PLUGIN_N_CB_SUBTYPES,
};
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index aa74e580bd..4785838eca 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -104,30 +104,6 @@ void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
void *userdata)
{ }
-static void gen_empty_udata_cb(void (*gen_helper)(TCGv_i32, TCGv_ptr))
-{
-TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
-TCGv_ptr udata = tcg_temp_ebb_new_ptr();
-
-tcg_gen_movi_ptr(udata, 0);
-tcg_gen_ld_i32(cpu_index, tcg_env,
- -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-gen_helper(cpu_index, udata);
-
-tcg_temp_free_ptr(udata);
-tcg_temp_free_i32(cpu_index);
-}
-
-static void gen_empty_udata_cb_no_wg(void)
-{
-gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_wg);
-}
-
-static void gen_empty_udata_cb_no_rwg(void)
-{
-gen_empty_udata_cb(gen_helper_plugin_vcpu_udata_cb_no_rwg);
-}
-
/*
* For now we only support addi_i64.
* When we support more ops, we can generate one empty inline cb for each.
@@ -176,51 +152,19 @@ static void gen_empty_mem_cb(TCGv_i64 addr, uint32_t info)
tcg_temp_free_i32(cpu_index);
}
-/*
- * Share the same function for enable/disable. When enabling, the NULL
- * pointer will be overwritten later.
- */
-static void gen_empty_mem_helper(void)
-{
-TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
-
-tcg_gen_movi_ptr(ptr, 0);
-tcg_gen_st_ptr(ptr, tcg_env, offsetof(CPUState, plugin_mem_cbs) -
- offsetof(ArchCPU, env));
-tcg_temp_free_ptr(ptr);
-}
-
static void gen_plugin_cb_start(enum plugin_gen_from from,
enum plugin_gen_cb type, unsigned wr)
{
tcg_gen_plugin_cb_start(from, type, wr);
}
-static void gen_wrapped(enum plugin_gen_from from,
-enum plugin_gen_cb type, void (*func)(void))
-{
-gen_plugin_cb_start(from, type, 0);
-func();
-tcg_gen_plugin_cb_end();
-}
-
static void plugin_gen_empty_callback(enum plugin_gen_from from)
{
switch (from) {
case PLUGIN_GEN_AFTER_INSN:
case PLUGIN_GEN_FROM_TB:
-tcg_gen_plugin_cb(from);
-break;
case PLUGIN_GEN_FROM_INSN:
-/*
- * Note: plugin_gen_inject() relies on ENABLE_MEM_HELPER being
- * the first callback of an instruction
- */
-gen_wrapped(from, PLUGIN_GEN_ENABLE_MEM_HELPER,
-gen_empty_mem_helper);
-gen_wrapped(from, PLUGIN_GEN_CB_UDATA, gen_empty_udata_cb_no_rwg);
-gen_wrapped(from, PLUGIN_GEN_CB_UDATA_R, gen_empty_udata_cb_no_wg);
-gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
+tcg_gen_plugin_cb(from);
break;
default:
g_assert_not_reached();
@@ -374,18 +318,6 @@ static TCGOp *copy_mul_i32(TCGOp **begin_op, TCGOp *op,
uint32_t v)
return op;
}
-static TCGOp *copy_st_ptr(TCGOp **begin_op, TCGOp *op)
-{
-if (UINTPTR_MAX == UINT32_MAX) {
-/* st_i32 */
-op = copy_op(begin_op, op, INDEX_op_st_i32);
-} else {
-/* st_i64 */
-op = copy_st_i64(begin_op, op);
-}
-return op;
-}
-
static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *func, int *cb_idx)
{
TCGOp *old_op;
@@ -409,32 +341,6 @@ static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void
*func, int *cb_idx)
return op;
}
-/*
- * When we append/replace ops here we are sensitive to changing patterns of
- * TCGOps generated by the tcg_gen_FOO calls when we generated the
- * empty callbacks. This will assert very quickly in a debug build as
- * we assert the ops we are replacing are the correct ones.
- */
-static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb,
- TCGOp *begin_op, TCGOp *op, int *cb_idx)
-{
-/* const_ptr */
-op = copy_const_ptr(_op, op, cb->userp);
-
-/* copy the ld_i32, but note that we only have to copy it once */
-if (*cb_idx == -1) {
-op = copy_op(_op, op, INDEX_op_ld_i32);
-} else {
-begin_op = QTAILQ_NEXT(begin_op, link);
-tcg_debug_assert(begin_op && begin_op->opc == INDEX_op_ld_i32);
-}
-
-/* call */
-op = copy_call(_op, op, cb->regular.f.vcpu_udata, cb_idx);
-
-return op;
-}
-
static TCGOp *append_inline_cb(const struct qemu_plugin_dyn_cb *cb,
TCGOp *begin_op, TCGOp *op,
[PATCH 09/22] plugins: Add PLUGIN_GEN_AFTER_TB
Delay test of plugin_tb->mem_helper until the inject pass.
Signed-off-by: Richard Henderson
---
accel/tcg/plugin-gen.c | 37 -
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index f92aa80510..aa74e580bd 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -75,6 +75,7 @@ enum plugin_gen_from {
PLUGIN_GEN_FROM_INSN,
PLUGIN_GEN_FROM_MEM,
PLUGIN_GEN_AFTER_INSN,
+PLUGIN_GEN_AFTER_TB,
PLUGIN_GEN_N_FROMS,
};
@@ -615,20 +616,9 @@ static void inject_mem_enable_helper(struct qemu_plugin_tb
*ptb,
/* called before finishing a TB with exit_tb, goto_tb or goto_ptr */
void plugin_gen_disable_mem_helpers(void)
{
-/*
- * We could emit the clearing unconditionally and be done. However, this
can
- * be wasteful if for instance plugins don't track memory accesses, or if
- * most TBs don't use helpers. Instead, emit the clearing iff the TB calls
- * helpers that might access guest memory.
- *
- * Note: we do not reset plugin_tb->mem_helper here; a TB might have
several
- * exit points, and we want to emit the clearing from all of them.
- */
-if (!tcg_ctx->plugin_tb->mem_helper) {
-return;
+if (tcg_ctx->plugin_insn) {
+tcg_gen_plugin_cb(PLUGIN_GEN_AFTER_TB);
}
-tcg_gen_st_ptr(tcg_constant_ptr(NULL), tcg_env,
- offsetof(CPUState, plugin_mem_cbs) - offsetof(ArchCPU,
env));
}
static void plugin_gen_insn_udata(const struct qemu_plugin_tb *ptb,
@@ -679,14 +669,11 @@ static void plugin_gen_enable_mem_helper(struct
qemu_plugin_tb *ptb,
inject_mem_enable_helper(ptb, insn, begin_op);
}
-static void gen_disable_mem_helper(struct qemu_plugin_tb *ptb,
- struct qemu_plugin_insn *insn)
+static void gen_disable_mem_helper(void)
{
-if (insn->mem_helper) {
-tcg_gen_st_ptr(tcg_constant_ptr(0), tcg_env,
- offsetof(CPUState, plugin_mem_cbs) -
- offsetof(ArchCPU, env));
-}
+tcg_gen_st_ptr(tcg_constant_ptr(0), tcg_env,
+ offsetof(CPUState, plugin_mem_cbs) -
+ offsetof(ArchCPU, env));
}
static void gen_udata_cb(struct qemu_plugin_dyn_cb *cb)
@@ -812,9 +799,17 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
tcg_ctx->emit_before_op = op;
switch (from) {
+case PLUGIN_GEN_AFTER_TB:
+if (plugin_tb->mem_helper) {
+gen_disable_mem_helper();
+}
+break;
+
case PLUGIN_GEN_AFTER_INSN:
assert(insn != NULL);
-gen_disable_mem_helper(plugin_tb, insn);
+if (insn->mem_helper) {
+gen_disable_mem_helper();
+}
break;
case PLUGIN_GEN_FROM_TB:
--
2.34.1
[PATCH 14/22] tcg: Remove INDEX_op_plugin_cb_{start,end}
These opcodes are no longer used.
Signed-off-by: Richard Henderson
---
include/tcg/tcg-op-common.h | 2 --
include/tcg/tcg-opc.h | 2 --
accel/tcg/plugin-gen.c | 18 --
tcg/tcg-op.c| 10 --
4 files changed, 32 deletions(-)
diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h
index 72b80b20d0..009e2778c5 100644
--- a/include/tcg/tcg-op-common.h
+++ b/include/tcg/tcg-op-common.h
@@ -76,8 +76,6 @@ void tcg_gen_lookup_and_goto_ptr(void);
void tcg_gen_plugin_cb(unsigned from);
void tcg_gen_plugin_mem_cb(TCGv_i64 addr, unsigned meminfo);
-void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr);
-void tcg_gen_plugin_cb_end(void);
/* 32 bit ops */
diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h
index be9e36e386..546eb49c11 100644
--- a/include/tcg/tcg-opc.h
+++ b/include/tcg/tcg-opc.h
@@ -199,8 +199,6 @@ DEF(goto_ptr, 0, 1, 0, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
DEF(plugin_cb, 0, 0, 1, TCG_OPF_NOT_PRESENT)
DEF(plugin_mem_cb, 0, 1, 1, TCG_OPF_NOT_PRESENT)
-DEF(plugin_cb_start, 0, 0, 3, TCG_OPF_NOT_PRESENT)
-DEF(plugin_cb_end, 0, 0, 0, TCG_OPF_NOT_PRESENT)
/* Replicate ld/st ops for 32 and 64-bit guest addresses. */
DEF(qemu_ld_a32_i32, 1, 1, 1,
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index b5261edc38..c8f0e0ecaa 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -58,29 +58,11 @@
# define CONFIG_SOFTMMU_GATE 0
#endif
-/*
- * plugin_cb_start TCG op args[]:
- * 0: enum plugin_gen_from
- * 1: enum plugin_gen_cb
- * 2: set to 1 for mem callback that is a write, 0 otherwise.
- */
-
enum plugin_gen_from {
PLUGIN_GEN_FROM_TB,
PLUGIN_GEN_FROM_INSN,
PLUGIN_GEN_AFTER_INSN,
PLUGIN_GEN_AFTER_TB,
-PLUGIN_GEN_N_FROMS,
-};
-
-enum plugin_gen_cb {
-PLUGIN_GEN_CB_UDATA,
-PLUGIN_GEN_CB_UDATA_R,
-PLUGIN_GEN_CB_INLINE,
-PLUGIN_GEN_CB_MEM,
-PLUGIN_GEN_ENABLE_MEM_HELPER,
-PLUGIN_GEN_DISABLE_MEM_HELPER,
-PLUGIN_GEN_N_CBS,
};
static void plugin_gen_empty_callback(enum plugin_gen_from from)
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 0ae12fa49d..eff3728622 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -322,16 +322,6 @@ void tcg_gen_plugin_mem_cb(TCGv_i64 addr, unsigned meminfo)
tcg_gen_op2(INDEX_op_plugin_mem_cb, tcgv_i64_arg(addr), meminfo);
}
-void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr)
-{
-tcg_gen_op3(INDEX_op_plugin_cb_start, from, type, wr);
-}
-
-void tcg_gen_plugin_cb_end(void)
-{
-tcg_emit_op(INDEX_op_plugin_cb_end, 0);
-}
-
/* 32 bit ops */
void tcg_gen_discard_i32(TCGv_i32 arg)
--
2.34.1
[PATCH 01/22] tcg: Add TCGContext.emit_before_op
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;
/* Tells which temporary holds a given register.
It does not take into account fixed registers */
diff --git a/tcg/tcg.c b/tcg/tcg.c
index d6670237fb..0c0bb9d169 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1521,6 +1521,7 @@ void tcg_func_start(TCGContext *s)
QTAILQ_INIT(>ops);
QTAILQ_INIT(>free_ops);
+s->emit_before_op = NULL;
QSIMPLEQ_INIT(>labels);
tcg_debug_assert(s->addr_type == TCG_TYPE_I32 ||
@@ -2332,7 +2333,11 @@ static void tcg_gen_callN(TCGHelperInfo *info, TCGTemp
*ret, TCGTemp **args)
op->args[pi++] = (uintptr_t)info;
tcg_debug_assert(pi == total_args);
-QTAILQ_INSERT_TAIL(_ctx->ops, op, link);
+if (tcg_ctx->emit_before_op) {
+QTAILQ_INSERT_BEFORE(tcg_ctx->emit_before_op, op, link);
+} else {
+QTAILQ_INSERT_TAIL(_ctx->ops, op, link);
+}
tcg_debug_assert(n_extend < ARRAY_SIZE(extend_free));
for (i = 0; i < n_extend; ++i) {
@@ -3215,7 +3220,12 @@ static TCGOp *tcg_op_alloc(TCGOpcode opc, unsigned nargs)
TCGOp *tcg_emit_op(TCGOpcode opc, unsigned nargs)
{
TCGOp *op = tcg_op_alloc(opc, nargs);
-QTAILQ_INSERT_TAIL(_ctx->ops, op, link);
+
+if (tcg_ctx->emit_before_op) {
+QTAILQ_INSERT_BEFORE(tcg_ctx->emit_before_op, op, link);
+} else {
+QTAILQ_INSERT_TAIL(_ctx->ops, op, link);
+}
return op;
}
--
2.34.1
[PATCH 11/22] plugins: Use emit_before_op for PLUGIN_GEN_FROM_MEM
Introduce a new plugin_mem_cb op to hold the address temp
and meminfo computed by tcg-op-ldst.c. Because this now
has its own opcode, we no longer need PLUGIN_GEN_FROM_MEM.
Signed-off-by: Richard Henderson
---
include/exec/plugin-gen.h | 4 -
include/tcg/tcg-op-common.h | 1 +
include/tcg/tcg-opc.h | 1 +
accel/tcg/plugin-gen.c | 408
tcg/tcg-op-ldst.c | 6 +-
tcg/tcg-op.c| 5 +
6 files changed, 54 insertions(+), 371 deletions(-)
diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
index c4552b5061..f333f33198 100644
--- a/include/exec/plugin-gen.h
+++ b/include/exec/plugin-gen.h
@@ -25,7 +25,6 @@ void plugin_gen_insn_start(CPUState *cpu, const struct
DisasContextBase *db);
void plugin_gen_insn_end(void);
void plugin_gen_disable_mem_helpers(void);
-void plugin_gen_empty_mem_callback(TCGv_i64 addr, uint32_t info);
#else /* !CONFIG_PLUGIN */
@@ -48,9 +47,6 @@ static inline void plugin_gen_tb_end(CPUState *cpu, size_t
num_insns)
static inline void plugin_gen_disable_mem_helpers(void)
{ }
-static inline void plugin_gen_empty_mem_callback(TCGv_i64 addr, uint32_t info)
-{ }
-
#endif /* CONFIG_PLUGIN */
#endif /* QEMU_PLUGIN_GEN_H */
diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h
index 9de5a7f280..72b80b20d0 100644
--- a/include/tcg/tcg-op-common.h
+++ b/include/tcg/tcg-op-common.h
@@ -75,6 +75,7 @@ void tcg_gen_goto_tb(unsigned idx);
void tcg_gen_lookup_and_goto_ptr(void);
void tcg_gen_plugin_cb(unsigned from);
+void tcg_gen_plugin_mem_cb(TCGv_i64 addr, unsigned meminfo);
void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr);
void tcg_gen_plugin_cb_end(void);
diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h
index 3b7cb2bce1..be9e36e386 100644
--- a/include/tcg/tcg-opc.h
+++ b/include/tcg/tcg-opc.h
@@ -198,6 +198,7 @@ DEF(goto_tb, 0, 0, 1, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
DEF(goto_ptr, 0, 1, 0, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
DEF(plugin_cb, 0, 0, 1, TCG_OPF_NOT_PRESENT)
+DEF(plugin_mem_cb, 0, 1, 1, TCG_OPF_NOT_PRESENT)
DEF(plugin_cb_start, 0, 0, 3, TCG_OPF_NOT_PRESENT)
DEF(plugin_cb_end, 0, 0, 0, TCG_OPF_NOT_PRESENT)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 4785838eca..be7fd548cc 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -73,7 +73,6 @@
enum plugin_gen_from {
PLUGIN_GEN_FROM_TB,
PLUGIN_GEN_FROM_INSN,
-PLUGIN_GEN_FROM_MEM,
PLUGIN_GEN_AFTER_INSN,
PLUGIN_GEN_AFTER_TB,
PLUGIN_GEN_N_FROMS,
@@ -104,60 +103,6 @@ void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
void *userdata)
{ }
-/*
- * For now we only support addi_i64.
- * When we support more ops, we can generate one empty inline cb for each.
- */
-static void gen_empty_inline_cb(void)
-{
-TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
-TCGv_ptr cpu_index_as_ptr = tcg_temp_ebb_new_ptr();
-TCGv_i64 val = tcg_temp_ebb_new_i64();
-TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
-
-tcg_gen_ld_i32(cpu_index, tcg_env,
- -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-/* second operand will be replaced by immediate value */
-tcg_gen_mul_i32(cpu_index, cpu_index, cpu_index);
-tcg_gen_ext_i32_ptr(cpu_index_as_ptr, cpu_index);
-
-tcg_gen_movi_ptr(ptr, 0);
-tcg_gen_add_ptr(ptr, ptr, cpu_index_as_ptr);
-tcg_gen_ld_i64(val, ptr, 0);
-/* second operand will be replaced by immediate value */
-tcg_gen_add_i64(val, val, val);
-
-tcg_gen_st_i64(val, ptr, 0);
-tcg_temp_free_ptr(ptr);
-tcg_temp_free_i64(val);
-tcg_temp_free_ptr(cpu_index_as_ptr);
-tcg_temp_free_i32(cpu_index);
-}
-
-static void gen_empty_mem_cb(TCGv_i64 addr, uint32_t info)
-{
-TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
-TCGv_i32 meminfo = tcg_temp_ebb_new_i32();
-TCGv_ptr udata = tcg_temp_ebb_new_ptr();
-
-tcg_gen_movi_i32(meminfo, info);
-tcg_gen_movi_ptr(udata, 0);
-tcg_gen_ld_i32(cpu_index, tcg_env,
- -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-
-gen_helper_plugin_vcpu_mem_cb(cpu_index, meminfo, addr, udata);
-
-tcg_temp_free_ptr(udata);
-tcg_temp_free_i32(meminfo);
-tcg_temp_free_i32(cpu_index);
-}
-
-static void gen_plugin_cb_start(enum plugin_gen_from from,
-enum plugin_gen_cb type, unsigned wr)
-{
-tcg_gen_plugin_cb_start(from, type, wr);
-}
-
static void plugin_gen_empty_callback(enum plugin_gen_from from)
{
switch (from) {
@@ -171,278 +116,6 @@ static void plugin_gen_empty_callback(enum
plugin_gen_from from)
}
}
-void plugin_gen_empty_mem_callback(TCGv_i64 addr, uint32_t info)
-{
-enum qemu_plugin_mem_rw rw = get_plugin_meminfo_rw(info);
-
-gen_plugin_cb_start(PLUGIN_GEN_FROM_MEM, PLUGIN_GEN_CB_MEM, rw);
-gen_empty_mem_cb(addr, info);
-tcg_gen_plugin_cb_end();
-
-
[PATCH 12/22] plugins: Remove plugin helpers
These placeholder helpers are no longer required.
Signed-off-by: Richard Henderson
---
accel/tcg/plugin-helpers.h | 5 -
include/exec/helper-gen-common.h | 4
include/exec/helper-proto-common.h | 4
accel/tcg/plugin-gen.c | 20
4 files changed, 33 deletions(-)
delete mode 100644 accel/tcg/plugin-helpers.h
diff --git a/accel/tcg/plugin-helpers.h b/accel/tcg/plugin-helpers.h
deleted file mode 100644
index 11796436f3..00
--- a/accel/tcg/plugin-helpers.h
+++ /dev/null
@@ -1,5 +0,0 @@
-#ifdef CONFIG_PLUGIN
-DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb_no_wg, TCG_CALL_NO_WG |
TCG_CALL_PLUGIN, void, i32, ptr)
-DEF_HELPER_FLAGS_2(plugin_vcpu_udata_cb_no_rwg, TCG_CALL_NO_RWG |
TCG_CALL_PLUGIN, void, i32, ptr)
-DEF_HELPER_FLAGS_4(plugin_vcpu_mem_cb, TCG_CALL_NO_RWG | TCG_CALL_PLUGIN,
void, i32, i32, i64, ptr)
-#endif
diff --git a/include/exec/helper-gen-common.h b/include/exec/helper-gen-common.h
index 5d6d78a625..834590dc4e 100644
--- a/include/exec/helper-gen-common.h
+++ b/include/exec/helper-gen-common.h
@@ -11,8 +11,4 @@
#include "exec/helper-gen.h.inc"
#undef HELPER_H
-#define HELPER_H "accel/tcg/plugin-helpers.h"
-#include "exec/helper-gen.h.inc"
-#undef HELPER_H
-
#endif /* HELPER_GEN_COMMON_H */
diff --git a/include/exec/helper-proto-common.h
b/include/exec/helper-proto-common.h
index 8b67170a22..16782ef46c 100644
--- a/include/exec/helper-proto-common.h
+++ b/include/exec/helper-proto-common.h
@@ -13,8 +13,4 @@
#include "exec/helper-proto.h.inc"
#undef HELPER_H
-#define HELPER_H "accel/tcg/plugin-helpers.h"
-#include "exec/helper-proto.h.inc"
-#undef HELPER_H
-
#endif /* HELPER_PROTO_COMMON_H */
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index be7fd548cc..b5261edc38 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -51,11 +51,6 @@
#include "exec/exec-all.h"
#include "exec/plugin-gen.h"
#include "exec/translator.h"
-#include "exec/helper-proto-common.h"
-
-#define HELPER_H "accel/tcg/plugin-helpers.h"
-#include "exec/helper-info.c.inc"
-#undef HELPER_H
#ifdef CONFIG_SOFTMMU
# define CONFIG_SOFTMMU_GATE 1
@@ -88,21 +83,6 @@ enum plugin_gen_cb {
PLUGIN_GEN_N_CBS,
};
-/*
- * These helpers are stubs that get dynamically switched out for calls
- * direct to the plugin if they are subscribed to.
- */
-void HELPER(plugin_vcpu_udata_cb_no_wg)(uint32_t cpu_index, void *udata)
-{ }
-
-void HELPER(plugin_vcpu_udata_cb_no_rwg)(uint32_t cpu_index, void *udata)
-{ }
-
-void HELPER(plugin_vcpu_mem_cb)(unsigned int vcpu_index,
-qemu_plugin_meminfo_t info, uint64_t vaddr,
-void *userdata)
-{ }
-
static void plugin_gen_empty_callback(enum plugin_gen_from from)
{
switch (from) {
--
2.34.1
[PATCH 03/22] tcg: Pass function pointer to tcg_gen_call*
For normal helpers, read the function pointer from the
structure earlier. For plugins, this will allow the
function pointer to come from elsewhere.
Signed-off-by: Richard Henderson
---
include/tcg/tcg.h | 21 +---
include/exec/helper-gen.h.inc | 24 ---
tcg/tcg.c | 45 +++
3 files changed, 52 insertions(+), 38 deletions(-)
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index a6e7df146a..95a7f4d010 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -847,19 +847,22 @@ typedef struct TCGTargetOpDef {
bool tcg_op_supported(TCGOpcode op);
-void tcg_gen_call0(TCGHelperInfo *, TCGTemp *ret);
-void tcg_gen_call1(TCGHelperInfo *, TCGTemp *ret, TCGTemp *);
-void tcg_gen_call2(TCGHelperInfo *, TCGTemp *ret, TCGTemp *, TCGTemp *);
-void tcg_gen_call3(TCGHelperInfo *, TCGTemp *ret, TCGTemp *,
+void tcg_gen_call0(void *func, TCGHelperInfo *, TCGTemp *ret);
+void tcg_gen_call1(void *func, TCGHelperInfo *, TCGTemp *ret, TCGTemp *);
+void tcg_gen_call2(void *func, TCGHelperInfo *, TCGTemp *ret,
TCGTemp *, TCGTemp *);
-void tcg_gen_call4(TCGHelperInfo *, TCGTemp *ret, TCGTemp *, TCGTemp *,
- TCGTemp *, TCGTemp *);
-void tcg_gen_call5(TCGHelperInfo *, TCGTemp *ret, TCGTemp *, TCGTemp *,
+void tcg_gen_call3(void *func, TCGHelperInfo *, TCGTemp *ret,
TCGTemp *, TCGTemp *, TCGTemp *);
-void tcg_gen_call6(TCGHelperInfo *, TCGTemp *ret, TCGTemp *, TCGTemp *,
+void tcg_gen_call4(void *func, TCGHelperInfo *, TCGTemp *ret,
TCGTemp *, TCGTemp *, TCGTemp *, TCGTemp *);
-void tcg_gen_call7(TCGHelperInfo *, TCGTemp *ret, TCGTemp *, TCGTemp *,
+void tcg_gen_call5(void *func, TCGHelperInfo *, TCGTemp *ret,
TCGTemp *, TCGTemp *, TCGTemp *, TCGTemp *, TCGTemp *);
+void tcg_gen_call6(void *func, TCGHelperInfo *, TCGTemp *ret,
+ TCGTemp *, TCGTemp *, TCGTemp *, TCGTemp *,
+ TCGTemp *, TCGTemp *);
+void tcg_gen_call7(void *func, TCGHelperInfo *, TCGTemp *ret,
+ TCGTemp *, TCGTemp *, TCGTemp *, TCGTemp *,
+ TCGTemp *, TCGTemp *, TCGTemp *);
TCGOp *tcg_emit_op(TCGOpcode opc, unsigned nargs);
void tcg_op_remove(TCGContext *s, TCGOp *op);
diff --git a/include/exec/helper-gen.h.inc b/include/exec/helper-gen.h.inc
index c009641517..f7eb59b6c1 100644
--- a/include/exec/helper-gen.h.inc
+++ b/include/exec/helper-gen.h.inc
@@ -14,7 +14,8 @@
extern TCGHelperInfo glue(helper_info_, name); \
static inline void glue(gen_helper_, name)(dh_retvar_decl0(ret))\
{ \
-tcg_gen_call0((helper_info_, name), dh_retvar(ret)); \
+tcg_gen_call0(glue(helper_info_,name).func, \
+ (helper_info_,name), dh_retvar(ret));\
}
#define DEF_HELPER_FLAGS_1(name, flags, ret, t1)\
@@ -22,7 +23,8 @@ extern TCGHelperInfo glue(helper_info_, name);
\
static inline void glue(gen_helper_, name)(dh_retvar_decl(ret) \
dh_arg_decl(t1, 1)) \
{ \
-tcg_gen_call1((helper_info_, name), dh_retvar(ret),\
+tcg_gen_call1(glue(helper_info_,name).func, \
+ (helper_info_,name), dh_retvar(ret), \
dh_arg(t1, 1)); \
}
@@ -31,7 +33,8 @@ extern TCGHelperInfo glue(helper_info_, name);
\
static inline void glue(gen_helper_, name)(dh_retvar_decl(ret) \
dh_arg_decl(t1, 1), dh_arg_decl(t2, 2)) \
{ \
-tcg_gen_call2((helper_info_, name), dh_retvar(ret),\
+tcg_gen_call2(glue(helper_info_,name).func, \
+ (helper_info_,name), dh_retvar(ret), \
dh_arg(t1, 1), dh_arg(t2, 2));\
}
@@ -40,7 +43,8 @@ extern TCGHelperInfo glue(helper_info_, name);
\
static inline void glue(gen_helper_, name)(dh_retvar_decl(ret) \
dh_arg_decl(t1, 1), dh_arg_decl(t2, 2), dh_arg_decl(t3, 3)) \
{ \
-tcg_gen_call3((helper_info_, name), dh_retvar(ret),\
+tcg_gen_call3(glue(helper_info_,name).func, \
+ (helper_info_,name), dh_retvar(ret), \
dh_arg(t1, 1), dh_arg(t2, 2), dh_arg(t3, 3)); \
}
@@ -50,7 +54,8 @@ static inline void glue(gen_helper_,
name)(dh_retvar_decl(ret) \
dh_arg_decl(t1, 1),
[PATCH 08/22] plugins: Use emit_before_op for PLUGIN_GEN_FROM_TB
By having the qemu_plugin_cb_flags be recorded in the TCGHelperInfo,
we no longer need to distinguish PLUGIN_CB_REGULAR from
PLUGIN_CB_REGULAR_R, so place all TB callbacks in the same queue.
Signed-off-by: Richard Henderson
---
accel/tcg/plugin-gen.c | 96 +-
plugins/api.c | 6 +--
2 files changed, 58 insertions(+), 44 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 8fa342b425..f92aa80510 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -207,6 +207,7 @@ static void plugin_gen_empty_callback(enum plugin_gen_from
from)
{
switch (from) {
case PLUGIN_GEN_AFTER_INSN:
+case PLUGIN_GEN_FROM_TB:
tcg_gen_plugin_cb(from);
break;
case PLUGIN_GEN_FROM_INSN:
@@ -216,8 +217,6 @@ static void plugin_gen_empty_callback(enum plugin_gen_from
from)
*/
gen_wrapped(from, PLUGIN_GEN_ENABLE_MEM_HELPER,
gen_empty_mem_helper);
-/* fall through */
-case PLUGIN_GEN_FROM_TB:
gen_wrapped(from, PLUGIN_GEN_CB_UDATA, gen_empty_udata_cb_no_rwg);
gen_wrapped(from, PLUGIN_GEN_CB_UDATA_R, gen_empty_udata_cb_no_wg);
gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
@@ -632,24 +631,6 @@ void plugin_gen_disable_mem_helpers(void)
offsetof(CPUState, plugin_mem_cbs) - offsetof(ArchCPU,
env));
}
-static void plugin_gen_tb_udata(const struct qemu_plugin_tb *ptb,
-TCGOp *begin_op)
-{
-inject_udata_cb(ptb->cbs[PLUGIN_CB_REGULAR], begin_op);
-}
-
-static void plugin_gen_tb_udata_r(const struct qemu_plugin_tb *ptb,
- TCGOp *begin_op)
-{
-inject_udata_cb(ptb->cbs[PLUGIN_CB_REGULAR_R], begin_op);
-}
-
-static void plugin_gen_tb_inline(const struct qemu_plugin_tb *ptb,
- TCGOp *begin_op)
-{
-inject_inline_cb(ptb->cbs[PLUGIN_CB_INLINE], begin_op, op_ok);
-}
-
static void plugin_gen_insn_udata(const struct qemu_plugin_tb *ptb,
TCGOp *begin_op, int insn_idx)
{
@@ -708,6 +689,41 @@ static void gen_disable_mem_helper(struct qemu_plugin_tb
*ptb,
}
}
+static void gen_udata_cb(struct qemu_plugin_dyn_cb *cb)
+{
+TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
+
+tcg_gen_ld_i32(cpu_index, tcg_env,
+ -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
+tcg_gen_call2(cb->regular.f.vcpu_udata, cb->regular.info, NULL,
+ tcgv_i32_temp(cpu_index),
+ tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
+tcg_temp_free_i32(cpu_index);
+}
+
+static void gen_inline_cb(struct qemu_plugin_dyn_cb *cb)
+{
+GArray *arr = cb->inline_insn.entry.score->data;
+size_t offset = cb->inline_insn.entry.offset;
+TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
+TCGv_i64 val = tcg_temp_ebb_new_i64();
+TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
+
+tcg_gen_ld_i32(cpu_index, tcg_env,
+ -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
+tcg_gen_muli_i32(cpu_index, cpu_index, g_array_get_element_size(arr));
+tcg_gen_ext_i32_ptr(ptr, cpu_index);
+tcg_temp_free_i32(cpu_index);
+
+tcg_gen_addi_ptr(ptr, ptr, (intptr_t)arr->data);
+tcg_gen_ld_i64(val, ptr, offset);
+tcg_gen_addi_i64(val, val, cb->inline_insn.imm);
+tcg_gen_st_i64(val, ptr, offset);
+
+tcg_temp_free_i64(val);
+tcg_temp_free_ptr(ptr);
+}
+
/* #define DEBUG_PLUGIN_GEN_OPS */
static void pr_ops(void)
{
@@ -786,6 +802,8 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
{
enum plugin_gen_from from = op->args[0];
struct qemu_plugin_insn *insn = NULL;
+const GArray *cbs;
+int i, n;
if (insn_idx >= 0) {
insn = g_ptr_array_index(plugin_tb->insns, insn_idx);
@@ -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;
+
default:
g_assert_not_reached();
}
@@ -813,25 +850,6 @@ static void
[PATCH 07/22] plugins: Use emit_before_op for PLUGIN_GEN_AFTER_INSN
Introduce a new plugin_cb op and migrate one operation.
By using emit_before_op, we do not need to emit opcodes
early and modify them later -- we can simply emit the
final set of opcodes once.
Signed-off-by: Richard Henderson
---
include/tcg/tcg-op-common.h | 1 +
include/tcg/tcg-opc.h | 1 +
accel/tcg/plugin-gen.c | 74 +
tcg/tcg-op.c| 5 +++
4 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h
index 2d932a515e..9de5a7f280 100644
--- a/include/tcg/tcg-op-common.h
+++ b/include/tcg/tcg-op-common.h
@@ -74,6 +74,7 @@ void tcg_gen_goto_tb(unsigned idx);
*/
void tcg_gen_lookup_and_goto_ptr(void);
+void tcg_gen_plugin_cb(unsigned from);
void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr);
void tcg_gen_plugin_cb_end(void);
diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h
index b80227fa1c..3b7cb2bce1 100644
--- a/include/tcg/tcg-opc.h
+++ b/include/tcg/tcg-opc.h
@@ -197,6 +197,7 @@ DEF(exit_tb, 0, 0, 1, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
DEF(goto_tb, 0, 0, 1, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
DEF(goto_ptr, 0, 1, 0, TCG_OPF_BB_EXIT | TCG_OPF_BB_END)
+DEF(plugin_cb, 0, 0, 1, TCG_OPF_NOT_PRESENT)
DEF(plugin_cb_start, 0, 0, 3, TCG_OPF_NOT_PRESENT)
DEF(plugin_cb_end, 0, 0, 0, TCG_OPF_NOT_PRESENT)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index c56f104aee..8fa342b425 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -207,8 +207,7 @@ static void plugin_gen_empty_callback(enum plugin_gen_from
from)
{
switch (from) {
case PLUGIN_GEN_AFTER_INSN:
-gen_wrapped(from, PLUGIN_GEN_DISABLE_MEM_HELPER,
-gen_empty_mem_helper);
+tcg_gen_plugin_cb(from);
break;
case PLUGIN_GEN_FROM_INSN:
/*
@@ -614,16 +613,6 @@ static void inject_mem_enable_helper(struct qemu_plugin_tb
*ptb,
inject_mem_helper(begin_op, arr);
}
-static void inject_mem_disable_helper(struct qemu_plugin_insn *plugin_insn,
- TCGOp *begin_op)
-{
-if (likely(!plugin_insn->mem_helper)) {
-rm_ops(begin_op);
-return;
-}
-inject_mem_helper(begin_op, NULL);
-}
-
/* called before finishing a TB with exit_tb, goto_tb or goto_ptr */
void plugin_gen_disable_mem_helpers(void)
{
@@ -709,11 +698,14 @@ static void plugin_gen_enable_mem_helper(struct
qemu_plugin_tb *ptb,
inject_mem_enable_helper(ptb, insn, begin_op);
}
-static void plugin_gen_disable_mem_helper(struct qemu_plugin_tb *ptb,
- TCGOp *begin_op, int insn_idx)
+static void gen_disable_mem_helper(struct qemu_plugin_tb *ptb,
+ struct qemu_plugin_insn *insn)
{
-struct qemu_plugin_insn *insn = g_ptr_array_index(ptb->insns, insn_idx);
-inject_mem_disable_helper(insn, begin_op);
+if (insn->mem_helper) {
+tcg_gen_st_ptr(tcg_constant_ptr(0), tcg_env,
+ offsetof(CPUState, plugin_mem_cbs) -
+ offsetof(ArchCPU, env));
+}
}
/* #define DEBUG_PLUGIN_GEN_OPS */
@@ -772,16 +764,49 @@ static void pr_ops(void)
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));
+
+QTAILQ_FOREACH_SAFE(op, _ctx->ops, link, next) {
switch (op->opc) {
case INDEX_op_insn_start:
insn_idx++;
break;
+
+case INDEX_op_plugin_cb:
+{
+enum plugin_gen_from from = op->args[0];
+struct qemu_plugin_insn *insn = NULL;
+
+if (insn_idx >= 0) {
+insn = g_ptr_array_index(plugin_tb->insns, insn_idx);
+}
+
+tcg_ctx->emit_before_op = op;
+
+switch (from) {
+case PLUGIN_GEN_AFTER_INSN:
+assert(insn != NULL);
+gen_disable_mem_helper(plugin_tb, insn);
+break;
+default:
+g_assert_not_reached();
+}
+
+tcg_ctx->emit_before_op = NULL;
+tcg_op_remove(tcg_ctx, op);
+break;
+}
+
case INDEX_op_plugin_cb_start:
{
enum plugin_gen_from from = op->args[0];
@@ -846,19 +871,6 @@ static void plugin_gen_inject(struct qemu_plugin_tb
*plugin_tb)
break;
}
-case PLUGIN_GEN_AFTER_INSN:
-{
-g_assert(insn_idx >= 0);
-
-switch (type) {
-case
[PATCH 00/22] plugins: Rewrite plugin code generation
As I mooted with Pierrick earlier this week:
(1) Add a (trivial) mechanism for emitting code into
the middle of the opcode sequence: tcg_ctx->emit_before_op.
(2) Rip out all of the "empty" generation and "copy"
to modify those sequences. Replace with regular code
generation once we know what values to place.
There's probably still more cleanup that could be done:
There seems to be a lot of artificial separation between
plugins and the rest of the code base, even between
plugins/api.c and plugins/core.c.
I suspect that all of plugins could be moved into the
build-once buckets.
r~
Richard Henderson (22):
tcg: Add TCGContext.emit_before_op
tcg: Make tcg/helper-info.h self-contained
tcg: Pass function pointer to tcg_gen_call*
plugins: Zero new qemu_plugin_dyn_cb entries
plugins: Move function pointer in qemu_plugin_dyn_cb
plugins: Create TCGHelperInfo for all out-of-line callbacks
plugins: Use emit_before_op for PLUGIN_GEN_AFTER_INSN
plugins: Use emit_before_op for PLUGIN_GEN_FROM_TB
plugins: Add PLUGIN_GEN_AFTER_TB
plugins: Use emit_before_op for PLUGIN_GEN_FROM_INSN
plugins: Use emit_before_op for PLUGIN_GEN_FROM_MEM
plugins: Remove plugin helpers
tcg: Remove TCG_CALL_PLUGIN
tcg: Remove INDEX_op_plugin_cb_{start,end}
plugins: Simplify callback queues
plugins: Introduce PLUGIN_CB_MEM_REGULAR
plugins: Replace pr_ops with a proper debug dump flag
plugins: Split out common cb expanders
plugins: Merge qemu_plugin_tb_insn_get to plugin-gen.c
plugins: Move qemu_plugin_insn_cleanup_fn to tcg.c
plugins: Inline plugin_gen_empty_callback
plugins: Update the documentation block for plugin-gen.c
accel/tcg/plugin-helpers.h |5 -
include/exec/helper-gen-common.h |4 -
include/exec/helper-proto-common.h |4 -
include/exec/plugin-gen.h |4 -
include/qemu/log.h |1 +
include/qemu/plugin.h | 79 +--
include/tcg/helper-info.h |3 +
include/tcg/tcg-op-common.h|4 +-
include/tcg/tcg-opc.h |4 +-
include/tcg/tcg.h | 27 +-
include/exec/helper-gen.h.inc | 24 +-
accel/tcg/plugin-gen.c | 1008 +++-
plugins/api.c | 26 +-
plugins/core.c | 61 +-
tcg/tcg-op-ldst.c |6 +-
tcg/tcg-op.c |8 +-
tcg/tcg.c | 104 ++-
util/log.c |4 +
18 files changed, 424 insertions(+), 952 deletions(-)
delete mode 100644 accel/tcg/plugin-helpers.h
--
2.34.1
Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue
在 2024/3/16 上午1:06, Richard Henderson 写道:
> On 3/14/24 23:01, lixianglai wrote:
>> Hi Richard :
>>
>>> On 3/14/24 20:22, lixianglai wrote:
Hi Richard:
> On 3/13/24 15:33, Xianglai Li wrote:
>> + if (unlikely((level == 0) || (level > 4))) {
>> + return base;
>> + }
>>> ...
> Perhaps it would be worthwhile to add another for the level==0 or > 4
> case above?
>
A normal level 4 page table should not print an error log,
only if a level 4 page is large, so we should put it in
if (FIELD_EX64(base, TLBENTRY, HUGE)) {
if (unlikely(level == 4)) {
qemu_log_mask(LOG_GUEST_ERROR,
"Attempted use of level %lu huge page\n",
level);
}
if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
return base;
} else {
return FIELD_DP64(base, TLBENTRY, LEVEL, level);
}
}
>>>
>>> A level 5 page table is not normal, nor is a level 0 lddir.
>>>
>>
>> We communicate with the hardware guys that the behavior above level 4 and
>> lddir 0 is undefined behavior.
>>
>> The result of our test on 3A5000 is that it has no any effect on "base",
>>
>> however in future chips the behavior may change since it may support 5-level
>> page table and width for level[13:14] may change also.
>>
>>
>> So in this context,I am not sure which level to use to print logs,
>>
>> which content to print, and where to add these prints,
>>
>> any more detailed advice?
>
> Yes, right there in the IF that I quoted at the top.
> What I was trying to spell out is
>
> if (unlikely(level == 0 || level > 4)) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "Attepted LDDIR with level %"PRId64"\n", level);
> return base;
> }
>
Thank you very much, I will modify it in V6 version of patch.
Thanks,
Xianglai.
>
> r~
Re: [PATCH-for-9.1 12/12] exec/poison: Poison CONFIG_SOFTMMU again
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote:
Now that the confusion around SOFTMMU vs SYSTEM emulation
was clarified, we can restore the CONFIG_SOFTMMU poison
pragma.
This reverts commit d31b84041d4353ef310ffde23c87b78c2aa32ead
("exec/poison: Do not poison CONFIG_SOFTMMU").
Signed-off-by: Philippe Mathieu-Daudé
---
include/exec/poison.h | 1 +
scripts/make-config-poison.sh | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/exec/poison.h b/include/exec/poison.h
index 1ea5633eb3..fbec710f6c 100644
--- a/include/exec/poison.h
+++ b/include/exec/poison.h
@@ -84,6 +84,7 @@
#pragma GCC poison CONFIG_HVF
#pragma GCC poison CONFIG_LINUX_USER
#pragma GCC poison CONFIG_KVM
+#pragma GCC poison CONFIG_SOFTMMU
#pragma GCC poison CONFIG_WHPX
#pragma GCC poison CONFIG_XEN
diff --git a/scripts/make-config-poison.sh b/scripts/make-config-poison.sh
index 2b36907e23..6ef5580f84 100755
--- a/scripts/make-config-poison.sh
+++ b/scripts/make-config-poison.sh
@@ -9,7 +9,6 @@ fi
exec sed -n \
-e' /CONFIG_TCG/d' \
-e '/CONFIG_USER_ONLY/d' \
- -e '/CONFIG_SOFTMMU/d' \
-e '/^#define / {' \
-e's///' \
-e's/ .*//' \
Reviewed-by: Richard Henderson
r~
Re: [PATCH-for-9.1 06/12] tcg/sparc64: Check for USER_ONLY definition instead of SOFTMMU one
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote: Since we *might* have user emulation with softmmu, replace the system emulation check by !user emulation one. Signed-off-by: Philippe Mathieu-Daudé --- tcg/sparc64/tcg-target.c.inc | 8 1 file changed, 4 insertions(+), 4 deletions(-) Having read further, the ultimate goal is worthwhile. It'll be easy to turn these into runtime tests when we get there. Reviewed-by: Richard Henderson r~
Re: [PATCH-for-9.1 11/12] tcg: Remove unused CONFIG_SOFTMMU definition from libtcg_system.fa
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé
---
tcg/meson.build | 1 -
1 file changed, 1 deletion(-)
diff --git a/tcg/meson.build b/tcg/meson.build
index 8251589fd4..b5246676c6 100644
--- a/tcg/meson.build
+++ b/tcg/meson.build
@@ -42,7 +42,6 @@ user_ss.add(tcg_user)
libtcg_system = static_library('tcg_system',
tcg_ss.sources() + genh,
name_suffix: 'fa',
-c_args: '-DCONFIG_SOFTMMU',
build_by_default: false)
tcg_system = declare_dependency(link_with: libtcg_system,
Reviewed-by: Richard Henderson
r~
Re: [PATCH-for-9.1 10/12] exec/cpu-defs: Restrict SOFTMMU specific definitions to accel/tcg/
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote: CPU_TLB_foo definitions are specific to SoftMMU and only used in accel/tcg/. Signed-off-by: Philippe Mathieu-Daudé --- accel/tcg/internal-target.h | 26 ++ include/exec/cpu-defs.h | 26 -- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/accel/tcg/internal-target.h b/accel/tcg/internal-target.h index b22b29c461..9b5cc9168b 100644 --- a/accel/tcg/internal-target.h +++ b/accel/tcg/internal-target.h @@ -12,6 +12,32 @@ #include "exec/exec-all.h" #include "exec/translate-all.h" +#if defined(CONFIG_SOFTMMU) && defined(CONFIG_TCG) I see this is moved intact, but drop the CONFIG_TCG ifdef within accel/tcg/. With that change, Reviewed-by: Richard Henderson r~
Re: [PATCH-for-9.1 09/12] accel/tcg/internal: Check for USER_ONLY definition instead of SOFTMMU
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote:
Since we *might* have user emulation with softmmu,
replace the system emulation check by !user emulation one.
Signed-off-by: Philippe Mathieu-Daudé
---
accel/tcg/internal-target.h | 6 +++---
accel/tcg/tb-hash.h | 4 ++--
accel/tcg/tcg-all.c | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/accel/tcg/internal-target.h b/accel/tcg/internal-target.h
index 4e36cf858e..b22b29c461 100644
--- a/accel/tcg/internal-target.h
+++ b/accel/tcg/internal-target.h
@@ -24,7 +24,7 @@
#define assert_memory_lock()
#endif
-#if defined(CONFIG_SOFTMMU) && defined(CONFIG_DEBUG_TCG)
+#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_DEBUG_TCG)
void assert_no_pages_locked(void);
#else
static inline void assert_no_pages_locked(void) { }
@@ -62,12 +62,12 @@ void tb_unlock_page1(tb_page_addr_t, tb_page_addr_t);
void tb_unlock_pages(TranslationBlock *);
#endif
-#ifdef CONFIG_SOFTMMU
+#ifndef CONFIG_USER_ONLY
void tb_invalidate_phys_range_fast(ram_addr_t ram_addr,
unsigned size,
uintptr_t retaddr);
G_NORETURN void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
-#endif /* CONFIG_SOFTMMU */
+#endif /* !CONFIG_USER_ONLY */
TranslationBlock *tb_gen_code(CPUState *cpu, vaddr pc,
uint64_t cs_base, uint32_t flags,
Ok.
diff --git a/accel/tcg/tb-hash.h b/accel/tcg/tb-hash.h
index a0c61f25cd..45a484ce82 100644
--- a/accel/tcg/tb-hash.h
+++ b/accel/tcg/tb-hash.h
@@ -25,7 +25,7 @@
#include "qemu/xxhash.h"
#include "tb-jmp-cache.h"
-#ifdef CONFIG_SOFTMMU
+#ifndef CONFIG_USER_ONLY
/* Only the bottom TB_JMP_PAGE_BITS of the jump cache hash bits vary for
addresses on the same page. The top bits are the same. This allows
@@ -58,7 +58,7 @@ static inline unsigned int tb_jmp_cache_hash_func(vaddr pc)
return (pc ^ (pc >> TB_JMP_CACHE_BITS)) & (TB_JMP_CACHE_SIZE - 1);
}
-#endif /* CONFIG_SOFTMMU */
+#endif /* CONFIG_USER_ONLY */
static inline
uint32_t tb_hash_func(tb_page_addr_t phys_pc, vaddr pc,
Not ok, this is really softmmu related. If we have user-only softmmu, then we'll need to
take multiple mappings into account, just like this.
Perhaps add a comment so it's easy to see this (and whichever else) have
already been audited?
diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index c6619f5b98..929af1f64c 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -116,7 +116,7 @@ static int tcg_init_machine(MachineState *ms)
tb_htable_init();
tcg_init(s->tb_size * MiB, s->splitwx_enabled, max_cpus);
-#if defined(CONFIG_SOFTMMU)
+#if !defined(CONFIG_USER_ONLY)
/*
* There's no guest base to take into account, so go ahead and
* initialize the prologue now.
With system, we *know* softmmu must be used.
With user-only, we will want to wait until after command-line processing.
Ok with comment change to "never a guest base".
r~
Re: [PATCH-for-9.1 07/12] plugins/api: Check for USER_ONLY definition instead of SOFTMMU one
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote: Since we*might* have user emulation with softmmu, replace the system emulation check by !user emulation one. Signed-off-by: Philippe Mathieu-Daudé --- plugins/api.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH-for-9.1 06/12] tcg/sparc64: Check for USER_ONLY definition instead of SOFTMMU one
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote:
Since we *might* have user emulation with softmmu,
replace the system emulation check by !user emulation one.
Signed-off-by: Philippe Mathieu-Daudé
---
tcg/sparc64/tcg-target.c.inc | 8
1 file changed, 4 insertions(+), 4 deletions(-)
This is all really softmmu.
If we ever grow user-only softmmu support, these will have to be runtime tests. But until
then the ifdefs are really pointing out softmmu uses.
r~
diff --git a/tcg/sparc64/tcg-target.c.inc b/tcg/sparc64/tcg-target.c.inc
index 176c98740b..56915a913b 100644
--- a/tcg/sparc64/tcg-target.c.inc
+++ b/tcg/sparc64/tcg-target.c.inc
@@ -78,7 +78,7 @@ static const char * const
tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
#define TCG_REG_T2 TCG_REG_G2
#define TCG_REG_T3 TCG_REG_O7
-#ifndef CONFIG_SOFTMMU
+#ifdef CONFIG_USER_ONLY
# define TCG_GUEST_BASE_REG TCG_REG_I5
#endif
@@ -961,7 +961,7 @@ static void tcg_target_qemu_prologue(TCGContext *s)
tcg_out32(s, SAVE | INSN_RD(TCG_REG_O6) | INSN_RS1(TCG_REG_O6) |
INSN_IMM13(-frame_size));
-#ifndef CONFIG_SOFTMMU
+#ifdef CONFIG_USER_ONLY
if (guest_base != 0) {
tcg_out_movi_int(s, TCG_TYPE_PTR, TCG_GUEST_BASE_REG,
guest_base, true, TCG_REG_T1);
@@ -1075,7 +1075,7 @@ static TCGLabelQemuLdst *prepare_host_addr(TCGContext *s,
HostAddress *h,
h->aa.align = MAX(h->aa.align, s_bits);
a_mask = (1u << h->aa.align) - 1;
-#ifdef CONFIG_SOFTMMU
+#ifndef CONFIG_USER_ONLY
int mem_index = get_mmuidx(oi);
int fast_off = tlb_mask_table_ofs(s, mem_index);
int mask_off = fast_off + offsetof(CPUTLBDescFast, mask);
@@ -1147,7 +1147,7 @@ static TCGLabelQemuLdst *prepare_host_addr(TCGContext *s,
HostAddress *h,
tcg_out_bpcc0(s, COND_NE, BPCC_PN | BPCC_ICC, 0);
}
h->base = guest_base ? TCG_GUEST_BASE_REG : TCG_REG_G0;
-#endif
+#endif /* CONFIG_USER_ONLY */
/* If the guest address must be zero-extended, do in the delay slot. */
if (addr_type == TCG_TYPE_I32) {
Re: [PATCH-for-9.0? 03/12] gdbstub: Correct invalid mentions of 'softmmu' by 'system'
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote: Signed-off-by: Philippe Mathieu-Daudé --- gdbstub/internals.h | 20 ++-- gdbstub/system.c| 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH-for-9.0? 04/12] gdbstub/system: Rename 'user_ctx' argument as 'ctx'
On 3/13/24 11:33, Philippe Mathieu-Daudé wrote: Signed-off-by: Philippe Mathieu-Daudé --- gdbstub/internals.h | 8 gdbstub/system.c| 8 2 files changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Richard Henderson r~
Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs
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?
>
> That's the only reason I didn't propose reverting commit decdc76772
> ("migration/multifd: Add mapped-ram support to fd: URI").
>
> For mapped-ram in libvirt, we'll definitely not use
> SOCKET_ADDRESS_TYPE_FD (as in the JSON), because I don't think libvirt
> supports the new API.
>
> As for SOCKET_ADDRESS_TYPE_FD as in "fd:", we could use it when
> direct-io is disabled. With direct-io, the fdset will be required.
>
> >
> > What about the old getfd? Is it obsolete because it relies on monitor
> > object? Or maybe it's still in use?
> >
> > It would be greatly helpful if there can be a summary of how libvirt uses
> > fd for migration purpose.
> >
> > Thanks,
>
--
Peter Xu
Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address
On 15/03/24 6:28 pm, Fabiano Rosas wrote:
Het Gala writes:
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.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3e8c19c4de..8806dc841e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
-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 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.
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 */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
qobject_unref(rsp);
-return result;
+return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who)
+{
+SocketAddress *addrs;
+char *connect_uri;
+
+addrs = migrate_get_socket_address(who);
+connect_uri = SocketAddress_to_str(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_uri;
}
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to);
}
qdict_put_str(args, "uri", uri ? uri : connect_uri);
Regards,
Het Gala
Re: [PATCH v4 2/2] vhost: Perform memory section dirty scans once per iteration
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.
-Siwei
?
Thanks
Re: [PATCH v4 1/2] vhost: dirty log should be per backend type
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.
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.
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, vhost_load_backend_state,
vhost_virtqueue_mask, vhost_config_mask, just to name a few. Why local
assert a problem?
Thanks,
-Siwei
Thanks
Re: [PATCH v3 for 9.1 1/6] virtio/virtio-pci: Handle extra notification data
On Fri, Mar 15, 2024 at 5:57 PM Jonah Palmer wrote:
>
> Add support to virtio-pci devices for handling the extra data sent
> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> transport feature has been negotiated.
>
> The extra data that's passed to the virtio-pci device when this
> feature is enabled varies depending on the device's virtqueue
> layout.
>
> In a split virtqueue layout, this data includes:
> - upper 16 bits: shadow_avail_idx
> - lower 16 bits: virtqueue index
>
> In a packed virtqueue layout, this data includes:
> - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> - lower 16 bits: virtqueue index
>
Reviewed-by: Eugenio Pérez
Thanks!
> Signed-off-by: Jonah Palmer
> ---
> hw/virtio/virtio-pci.c | 11 ---
> hw/virtio/virtio.c | 18 ++
> include/hw/virtio/virtio.h | 2 ++
> 3 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index cb6940fc0e..f3e0a08f53 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t
> addr, uint32_t val)
> {
> VirtIOPCIProxy *proxy = opaque;
> VirtIODevice *vdev = virtio_bus_get_device(>bus);
> -uint16_t vector;
> +uint16_t vector, vq_idx;
> hwaddr pa;
>
> switch (addr) {
> @@ -408,8 +408,13 @@ static void virtio_ioport_write(void *opaque, uint32_t
> addr, uint32_t val)
> vdev->queue_sel = val;
> break;
> case VIRTIO_PCI_QUEUE_NOTIFY:
> -if (val < VIRTIO_QUEUE_MAX) {
> -virtio_queue_notify(vdev, val);
> +vq_idx = val;
> +if (vq_idx < VIRTIO_QUEUE_MAX && virtio_queue_get_num(vdev, vq_idx))
> {
> +if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> +virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev,
> vq_idx),
> + val >> 16);
> +}
> +virtio_queue_notify(vdev, vq_idx);
> }
> break;
> case VIRTIO_PCI_STATUS:
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d229755eae..463426ca92 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n,
> int align)
> }
> }
>
> +void virtio_queue_set_shadow_avail_idx(VirtQueue *vq, uint16_t
> shadow_avail_idx)
> +{
> +if (!vq->vring.desc) {
> +return;
> +}
> +
> +/*
> + * 16-bit data for packed VQs include 1-bit wrap counter and
> + * 15-bit shadow_avail_idx.
> + */
> +if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +vq->shadow_avail_wrap_counter = (shadow_avail_idx >> 15) & 0x1;
> +vq->shadow_avail_idx = shadow_avail_idx & 0x7FFF;
> +} else {
> +vq->shadow_avail_idx = shadow_avail_idx;
> +}
> +}
> +
> static void virtio_queue_notify_vq(VirtQueue *vq)
> {
> if (vq->vring.desc && vq->handle_output) {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c8f72850bc..cdd4f86b61 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -306,6 +306,8 @@ int virtio_queue_ready(VirtQueue *vq);
>
> int virtio_queue_empty(VirtQueue *vq);
>
> +void virtio_queue_set_shadow_avail_idx(VirtQueue *vq, uint16_t idx);
> +
> /* Host binding interface. */
>
> uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr);
> --
> 2.39.3
>
Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs
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.
That's the only reason I didn't propose reverting commit decdc76772
("migration/multifd: Add mapped-ram support to fd: URI").
For mapped-ram in libvirt, we'll definitely not use
SOCKET_ADDRESS_TYPE_FD (as in the JSON), because I don't think libvirt
supports the new API.
As for SOCKET_ADDRESS_TYPE_FD as in "fd:", we could use it when
direct-io is disabled. With direct-io, the fdset will be required.
>
> What about the old getfd? Is it obsolete because it relies on monitor
> object? Or maybe it's still in use?
>
> It would be greatly helpful if there can be a summary of how libvirt uses
> fd for migration purpose.
>
> Thanks,
Re: [PATCH v2] vfio/pci: migration: Skip config space check for vendor specific capability during restore/load
On 11/03/24 8:32 pm, Alex Williamson wrote:
External email: Use caution opening links or attachments
On Mon, 11 Mar 2024 17:45:19 +0530
Vinayak Kale wrote:
In case of migration, during restore operation, qemu checks config space of the
pci device with the config space in the migration stream captured during save
operation. In case of config space data mismatch, restore operation is failed.
config space check is done in function get_pci_config_device(). By default VSC
(vendor-specific-capability) in config space is checked.
Ideally qemu should not check VSC for VFIO-PCI device during restore/load as
qemu is not aware of VSC ABI.
It's disappointing that we can't seem to have a discussion about why
it's not the responsibility of the underlying migration support in the
vfio-pci variant driver to make the vendor specific capability
consistent across migration.
I think it is device vendor driver's responsibility to ensure that VSC
is consistent across migration. Here consistency could mean that VSC
format should be same on source and destination, however actual VSC
contents may not be byte-to-byte identical.
If a vfio-pci device is migration capable and if vfio-pci vendor driver
is OK with volatile VSC contents as long as consistency is maintained
for VSC format then QEMU should exempt config space check for VSC contents.
Also, for future maintenance, specifically what device is currently
broken by this and under what conditions?
Under certain conditions VSC contents vary for NVIDIA vGPU devices in
case of live migration. Due to QEMU's current config space check for
VSC, live migration is broken across NVIDIA vGPU devices.
This patch skips the check for VFIO-PCI device by clearing pdev->cmask[] for VSC
offsets. If cmask[] is not set for an offset, then qemu skips config space check
for that offset.
Signed-off-by: Vinayak Kale
---
Version History
v1->v2:
- Limited scope of change to vfio-pci devices instead of all pci devices.
hw/vfio/pci.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d7fe06715c..9edaff4b37 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2132,6 +2132,22 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev,
uint8_t pos)
}
}
+static int vfio_add_vendor_specific_cap(VFIOPCIDevice *vdev, int pos,
+uint8_t size, Error **errp)
+{
+PCIDevice *pdev = >pdev;
+
+pos = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, size, errp);
+if (pos < 0) {
+return pos;
+}
+
+/* Exempt config space check for VSC during restore/load */
+memset(pdev->cmask + pos, 0, size);
This excludes the entire capability from comparison, including the
capability ID, next pointer, and capability length. Even if the
contents of the capability are considered volatile vendor information,
the header is spec defined ABI which must be consistent. Thanks,
This makes sense, I'll address this in V3. Thanks.
Alex
+
+return pos;
+}
+
static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
{
PCIDevice *pdev = >pdev;
@@ -2199,6 +2215,9 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t
pos, Error **errp)
vfio_check_af_flr(vdev, pos);
ret = pci_add_capability(pdev, cap_id, pos, size, errp);
break;
+case PCI_CAP_ID_VNDR:
+ret = vfio_add_vendor_specific_cap(vdev, pos, size, errp);
+break;
default:
ret = pci_add_capability(pdev, cap_id, pos, size, errp);
break;
Re: [PATCH 1/1] cxl/mem: Fix for the index of Clear Event Record Handle
Yuquan Wang wrote:
> The dev_dbg info for Clear Event Records mailbox command would report
> the handle of the next record to clear not the current one.
>
> This was because the index 'i' had incremented before printing the
> current handle value.
>
> This fix also adjusts the index variable name from 'i' to 'clear_cnt'
> which can be easier for developers to distinguish and understand.
>
> Signed-off-by: Yuquan Wang
> ---
> drivers/cxl/core/mbox.c | 21 +++--
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9adda4795eb7..3ca2845ae6aa 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -881,7 +881,7 @@ static int cxl_clear_event_record(struct cxl_memdev_state
> *mds,
> struct cxl_mbox_cmd mbox_cmd;
> u16 cnt;
> int rc = 0;
> - int i;
> + int clear_cnt;
>
> /* Payload size may limit the max handles */
> if (pl_size > mds->payload_size) {
> @@ -908,28 +908,29 @@ static int cxl_clear_event_record(struct
> cxl_memdev_state *mds,
>* Clear Event Records uses u8 for the handle cnt while Get Event
>* Record can return up to 0x records.
>*/
> - i = 0;
> + clear_cnt = 0;
> for (cnt = 0; cnt < total; cnt++) {
> struct cxl_event_record_raw *raw = _pl->records[cnt];
> struct cxl_event_generic *gen = >event.generic;
>
> - payload->handles[i++] = gen->hdr.handle;
> + payload->handles[clear_cnt] = gen->hdr.handle;
> dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
> - le16_to_cpu(payload->handles[i]));
Couldn't this patch be much smaller by just changing this to "i - 1",
because from the description the only problem is the dev_dbg()
statement, so just fix that.
Re: [PATCH v4 21/23] qapi/schema: add type hints
On Fri, Mar 15, 2024, 10:03 AM Markus Armbruster wrote: > John Snow writes: > > > This patch only adds type hints, which aren't utilized at runtime and > > don't change the behavior of this module in any way. > > > > In a scant few locations, type hints are removed where no longer > > necessary due to inference power from typing all of the rest of > > creation; and any type hints that no longer need string quotes are > > changed. > > > > Signed-off-by: John Snow > > --- > > scripts/qapi/schema.py | 568 - > > 1 file changed, 396 insertions(+), 172 deletions(-) > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index 3b8c2ebbb5f..d2faaea6eac 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > [...] > > > @@ -1006,18 +1181,27 @@ def _def_definition(self, defn): > > defn.info, "%s is already defined" % > other_defn.describe()) > > self._entity_dict[defn.name] = defn > > > > -def lookup_entity(self, name, typ=None): > > +def lookup_entity( > > +self, > > +name: str, > > +typ: Optional[type] = None, > > +) -> Optional[QAPISchemaEntity]: > > Optional[QAPISchemaDefinition], actually. > Ah! Very good catch. > > ent = self._entity_dict.get(name) > > if typ and not isinstance(ent, typ): > > return None > > return ent > > [...] > >
Re: [PATCH v2] target/s390x: improve cpu compatibility check error message
On 3/15/24 17:59, Nina Schoetterl-Glausch wrote:
> On Thu, 2024-03-14 at 22:37 +0100, Claudio Fontana wrote:
>> some users were confused by this message showing under TCG:
>>
>> Selected CPU generation is too new. Maximum supported model
>> in the configuration: 'xyz'
>>
>> Clarify that the maximum can depend on the accel, and add a
>> hint to try a different one.
>>
>> Also add a hint for features mismatch to suggest trying
>> different accel, QEMU and kernel versions.
>>
>> Signed-off-by: Claudio Fontana
>
> Reviewed-by: Nina Schoetterl-Glausch
>
>> ---
>> target/s390x/cpu_models.c | 22 +++---
>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 1a1c096122..8ed3bb6a27 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -500,6 +500,16 @@ static void error_prepend_missing_feat(const char
>> *name, void *opaque)
>> error_prepend((Error **) opaque, "%s ", name);
>> }
>>
>> +static void check_compat_model_failed(Error **errp,
>> + const S390CPUModel *max_model,
>> + const char *msg)
>> +{
>> +error_setg(errp, "%s. Maximum supported model in the current
>> configuration: \'%s\'",
>> + msg, max_model->def->name);
>> +error_append_hint(errp, "Consider a different accelerator, try \"-accel
>> help\"\n");
>> +return;
>> +}
>> +
>> static void check_compatibility(const S390CPUModel *max_model,
>> const S390CPUModel *model, Error **errp)
>> {
>> @@ -507,15 +517,11 @@ static void check_compatibility(const S390CPUModel
>> *max_model,
>> S390FeatBitmap missing;
>>
>> if (model->def->gen > max_model->def->gen) {
>> -error_setg(errp, "Selected CPU generation is too new. Maximum "
>> - "supported model in the configuration: \'%s\'",
>> - max_model->def->name);
>> +check_compat_model_failed(errp, max_model, "Selected CPU generation
>> is too new");
>> return;
>> } else if (model->def->gen == max_model->def->gen &&
>> model->def->ec_ga > max_model->def->ec_ga) {
>> -error_setg(errp, "Selected CPU GA level is too new. Maximum "
>> - "supported model in the configuration: \'%s\'",
>> - max_model->def->name);
>> +check_compat_model_failed(errp, max_model, "Selected CPU GA level
>> is too new");
>> return;
>> }
>>
>> @@ -537,7 +543,9 @@ static void check_compatibility(const S390CPUModel
>> *max_model,
>> error_setg(errp, " ");
>> s390_feat_bitmap_to_ascii(missing, errp, error_prepend_missing_feat);
>> error_prepend(errp, "Some features requested in the CPU model are not "
>> - "available in the configuration: ");
>> + "available in the current configuration: ");
>> +error_append_hint(errp,
>> + "Consider a different accelerator, QEMU, or kernel
>> version\n");
>
> If I'm reading the regex right, this ^, a string literal on a separate line,
> is excluded
> from the line length check.
Thanks, it's very good to know!
Claudio
>
>> }
>>
>> S390CPUModel *get_max_cpu_model(Error **errp)
>
Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue
On 3/14/24 23:01, lixianglai wrote:
Hi Richard :
On 3/14/24 20:22, lixianglai wrote:
Hi Richard:
On 3/13/24 15:33, Xianglai Li wrote:
+ if (unlikely((level == 0) || (level > 4))) {
+ return base;
+ }
...
Perhaps it would be worthwhile to add another for the level==0 or > 4 case
above?
A normal level 4 page table should not print an error log,
only if a level 4 page is large, so we should put it in
if (FIELD_EX64(base, TLBENTRY, HUGE)) {
if (unlikely(level == 4)) {
qemu_log_mask(LOG_GUEST_ERROR,
"Attempted use of level %lu huge page\n", level);
}
if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
return base;
} else {
return FIELD_DP64(base, TLBENTRY, LEVEL, level);
}
}
A level 5 page table is not normal, nor is a level 0 lddir.
We communicate with the hardware guys that the behavior above level 4 and lddir 0 is
undefined behavior.
The result of our test on 3A5000 is that it has no any effect on "base",
however in future chips the behavior may change since it may support 5-level page table
and width for level[13:14] may change also.
So in this context,I am not sure which level to use to print logs,
which content to print, and where to add these prints,
any more detailed advice?
Yes, right there in the IF that I quoted at the top.
What I was trying to spell out is
if (unlikely(level == 0 || level > 4)) {
qemu_log_mask(LOG_GUEST_ERROR,
"Attepted LDDIR with level %"PRId64"\n", level);
return base;
}
r~
Re: [PATCH for-9.0 v3] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Fri, Mar 15, 2024 at 04:59:49PM +0100, Kevin Wolf wrote:
VDUSE requires that virtqueues are first enabled before the DRIVER_OK
status flag is set; with the current API of the kernel module, it is
impossible to enable the opposite order in our block export code because
userspace is not notified when a virtqueue is enabled.
This requirement also mathces the normal initialisation order as done by
the generic vhost code in QEMU. However, commit 6c482547 accidentally
changed the order for vdpa-dev and broke access to VDUSE devices with
this.
This changes vdpa-dev to use the normal order again and use the standard
vhost callback .vhost_set_vring_enable for this. VDUSE devices can be
used with vdpa-dev again after this fix.
vhost_net intentionally avoided enabling the vrings for vdpa and does
this manually later while it does enable them for other vhost backends.
Reflect this in the vhost_net code and return early for vdpa, so that
the behaviour doesn't change for this device.
Cc: qemu-sta...@nongnu.org
Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
Signed-off-by: Kevin Wolf
---
v2:
- Actually make use of the @enable parameter
- Change vhost_net to preserve the current behaviour
v3:
- Updated trace point [Stefano]
- Fixed typo in comment [Stefano]
hw/net/vhost_net.c | 10 ++
hw/virtio/vdpa-dev.c | 5 +
hw/virtio/vhost-vdpa.c | 29 ++---
hw/virtio/vhost.c | 8 +++-
hw/virtio/trace-events | 2 +-
5 files changed, 45 insertions(+), 9 deletions(-)
LGTM!
Reviewed-by: Stefano Garzarella
Thanks,
Stefano
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e8e1661646..fd1a93701a 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable)
VHostNetState *net = get_vhost_net(nc);
const VhostOps *vhost_ops = net->dev.vhost_ops;
+/*
+ * vhost-vdpa network devices need to enable dataplane virtqueues after
+ * DRIVER_OK, so they can recover device state before starting dataplane.
+ * Because of that, we don't enable virtqueues here and leave it to
+ * net/vhost-vdpa.c.
+ */
+if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+return 0;
+}
+
nc->vring_enable = enable;
if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index eb9ecea83b..13e87f06f6 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev,
Error **errp)
s->dev.acked_features = vdev->guest_features;
-ret = vhost_dev_start(>dev, vdev, false);
+ret = vhost_dev_start(>dev, vdev, true);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error starting vhost");
goto err_guest_notifiers;
}
-for (i = 0; i < s->dev.nvqs; ++i) {
-vhost_vdpa_set_vring_ready(>vdpa, i);
-}
s->started = true;
/*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ddae494ca8..31453466af 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -886,19 +886,41 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev,
int idx)
return idx;
}
-int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned idx,
+ int enable)
{
struct vhost_dev *dev = v->dev;
struct vhost_vring_state state = {
.index = idx,
-.num = 1,
+.num = enable,
};
int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, );
-trace_vhost_vdpa_set_vring_ready(dev, idx, r);
+trace_vhost_vdpa_set_vring_enable_one(dev, idx, enable, r);
return r;
}
+static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable)
+{
+struct vhost_vdpa *v = dev->opaque;
+unsigned int i;
+int ret;
+
+for (i = 0; i < dev->nvqs; ++i) {
+ret = vhost_vdpa_set_vring_enable_one(v, i, enable);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
+
+int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+{
+return vhost_vdpa_set_vring_enable_one(v, idx, 1);
+}
+
static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
int fd)
{
@@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = {
.vhost_set_features = vhost_vdpa_set_features,
.vhost_reset_device = vhost_vdpa_reset_device,
.vhost_get_vq_index = vhost_vdpa_get_vq_index,
+.vhost_set_vring_enable = vhost_vdpa_set_vring_enable,
.vhost_get_config = vhost_vdpa_get_config,
.vhost_set_config = vhost_vdpa_set_config,
.vhost_requires_shm_log = NULL,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..a66186 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1984,7 +1984,13 @@ static int
Re: [PATCH v2] target/s390x: improve cpu compatibility check error message
On Thu, 2024-03-14 at 22:37 +0100, Claudio Fontana wrote:
> some users were confused by this message showing under TCG:
>
> Selected CPU generation is too new. Maximum supported model
> in the configuration: 'xyz'
>
> Clarify that the maximum can depend on the accel, and add a
> hint to try a different one.
>
> Also add a hint for features mismatch to suggest trying
> different accel, QEMU and kernel versions.
>
> Signed-off-by: Claudio Fontana
Reviewed-by: Nina Schoetterl-Glausch
> ---
> target/s390x/cpu_models.c | 22 +++---
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 1a1c096122..8ed3bb6a27 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -500,6 +500,16 @@ static void error_prepend_missing_feat(const char *name,
> void *opaque)
> error_prepend((Error **) opaque, "%s ", name);
> }
>
> +static void check_compat_model_failed(Error **errp,
> + const S390CPUModel *max_model,
> + const char *msg)
> +{
> +error_setg(errp, "%s. Maximum supported model in the current
> configuration: \'%s\'",
> + msg, max_model->def->name);
> +error_append_hint(errp, "Consider a different accelerator, try \"-accel
> help\"\n");
> +return;
> +}
> +
> static void check_compatibility(const S390CPUModel *max_model,
> const S390CPUModel *model, Error **errp)
> {
> @@ -507,15 +517,11 @@ static void check_compatibility(const S390CPUModel
> *max_model,
> S390FeatBitmap missing;
>
> if (model->def->gen > max_model->def->gen) {
> -error_setg(errp, "Selected CPU generation is too new. Maximum "
> - "supported model in the configuration: \'%s\'",
> - max_model->def->name);
> +check_compat_model_failed(errp, max_model, "Selected CPU generation
> is too new");
> return;
> } else if (model->def->gen == max_model->def->gen &&
> model->def->ec_ga > max_model->def->ec_ga) {
> -error_setg(errp, "Selected CPU GA level is too new. Maximum "
> - "supported model in the configuration: \'%s\'",
> - max_model->def->name);
> +check_compat_model_failed(errp, max_model, "Selected CPU GA level is
> too new");
> return;
> }
>
> @@ -537,7 +543,9 @@ static void check_compatibility(const S390CPUModel
> *max_model,
> error_setg(errp, " ");
> s390_feat_bitmap_to_ascii(missing, errp, error_prepend_missing_feat);
> error_prepend(errp, "Some features requested in the CPU model are not "
> - "available in the configuration: ");
> + "available in the current configuration: ");
> +error_append_hint(errp,
> + "Consider a different accelerator, QEMU, or kernel
> version\n");
If I'm reading the regex right, this ^, a string literal on a separate line, is
excluded
from the line length check.
> }
>
> S390CPUModel *get_max_cpu_model(Error **errp)
[PATCH v3 for 9.1 6/6] virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
Extend the virtio device property definitions to include the
VIRTIO_F_NOTIFICATION_DATA feature.
The default state of this feature is disabled, allowing it to be
explicitly enabled where it's supported.
Tested-by: Lei Yang
Reviewed-by: Eugenio Pérez
Signed-off-by: Jonah Palmer
---
include/hw/virtio/virtio.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index cdd4f86b61..14858c0924 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -370,7 +370,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
DEFINE_PROP_BIT64("packed", _state, _field, \
VIRTIO_F_RING_PACKED, false), \
DEFINE_PROP_BIT64("queue_reset", _state, _field, \
- VIRTIO_F_RING_RESET, true)
+ VIRTIO_F_RING_RESET, true), \
+DEFINE_PROP_BIT64("notification_data", _state, _field, \
+ VIRTIO_F_NOTIFICATION_DATA, false)
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
--
2.39.3
[PATCH v3 for 9.1 5/6] vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
Add support for the VIRTIO_F_NOTIFICATION_DATA feature across a variety
of vhost devices.
The inclusion of VIRTIO_F_NOTIFICATION_DATA in the feature bits arrays
for these devices ensures that the backend is capable of offering and
providing support for this feature, and that it can be disabled if the
backend does not support it.
Tested-by: Lei Yang
Reviewed-by: Eugenio Pérez
Signed-off-by: Jonah Palmer
---
hw/block/vhost-user-blk.c| 1 +
hw/net/vhost_net.c | 2 ++
hw/scsi/vhost-scsi.c | 1 +
hw/scsi/vhost-user-scsi.c| 1 +
hw/virtio/vhost-user-fs.c| 2 +-
hw/virtio/vhost-user-vsock.c | 1 +
net/vhost-vdpa.c | 1 +
7 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 6a856ad51a..983c0657da 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -51,6 +51,7 @@ static const int user_feature_bits[] = {
VIRTIO_F_RING_PACKED,
VIRTIO_F_IOMMU_PLATFORM,
VIRTIO_F_RING_RESET,
+VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e8e1661646..bb1f975b39 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -48,6 +48,7 @@ static const int kernel_feature_bits[] = {
VIRTIO_F_IOMMU_PLATFORM,
VIRTIO_F_RING_PACKED,
VIRTIO_F_RING_RESET,
+VIRTIO_F_NOTIFICATION_DATA,
VIRTIO_NET_F_HASH_REPORT,
VHOST_INVALID_FEATURE_BIT
};
@@ -55,6 +56,7 @@ static const int kernel_feature_bits[] = {
/* Features supported by others. */
static const int user_feature_bits[] = {
VIRTIO_F_NOTIFY_ON_EMPTY,
+VIRTIO_F_NOTIFICATION_DATA,
VIRTIO_RING_F_INDIRECT_DESC,
VIRTIO_RING_F_EVENT_IDX,
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index ae26bc19a4..3d5fe0994d 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -38,6 +38,7 @@ static const int kernel_feature_bits[] = {
VIRTIO_RING_F_EVENT_IDX,
VIRTIO_SCSI_F_HOTPLUG,
VIRTIO_F_RING_RESET,
+VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index a63b1f4948..0b050805a8 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -36,6 +36,7 @@ static const int user_feature_bits[] = {
VIRTIO_RING_F_EVENT_IDX,
VIRTIO_SCSI_F_HOTPLUG,
VIRTIO_F_RING_RESET,
+VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index cca2cd41be..ae48cc1c96 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -33,7 +33,7 @@ static const int user_feature_bits[] = {
VIRTIO_F_RING_PACKED,
VIRTIO_F_IOMMU_PLATFORM,
VIRTIO_F_RING_RESET,
-
+VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 9431b9792c..802b44a07d 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -21,6 +21,7 @@ static const int user_feature_bits[] = {
VIRTIO_RING_F_INDIRECT_DESC,
VIRTIO_RING_F_EVENT_IDX,
VIRTIO_F_NOTIFY_ON_EMPTY,
+VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 2a9ddb4552..5583ce5279 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -61,6 +61,7 @@ const int vdpa_feature_bits[] = {
VIRTIO_F_RING_PACKED,
VIRTIO_F_RING_RESET,
VIRTIO_F_VERSION_1,
+VIRTIO_F_NOTIFICATION_DATA,
VIRTIO_NET_F_CSUM,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
VIRTIO_NET_F_CTRL_MAC_ADDR,
--
2.39.3
[PATCH v3 for 9.1 0/6] virtio, vhost: Add VIRTIO_F_NOTIFICATION_DATA support
The goal of these patches are to add support to a variety of virtio and vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This feature indicates that a driver will pass extra data (instead of just a virtqueue's index) when notifying the corresponding device. The data passed in by the driver when this feature is enabled varies in format depending on if the device is using a split or packed virtqueue layout: Split VQ - Upper 16 bits: shadow_avail_idx - Lower 16 bits: virtqueue index Packed VQ - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx - Lower 16 bits: virtqueue index Also, due to the limitations of ioeventfd not being able to carry the extra provided by the driver, having both VIRTIO_F_NOTIFICATION_DATA feature and ioeventfd enabled is a functional mismatch. The user must explicitly disable ioeventfd for the device in the Qemu arguments when using this feature, else the device will fail to complete realization. For example, a device must explicitly enable notification_data as well as disable ioeventfd: -device virtio-scsi-pci,...,ioeventfd=off,notification_data=on A significant aspect of this effort has been to maintain compatibility across different backends. As such, the feature is offered by backend devices only when supported, with fallback mechanisms where backend support is absent. v3: Validate VQ idx via. virtio_queue_get_num() (pci, mmio, ccw) Rename virtio_queue_set_shadow_avail_data Only pass in upper 16 bits of 32-bit extra data (was redundant) Make notification compatibility check function static Drop tags on patches 1/6, 3/6, and 4/6 v2: Don't disable ioeventfd by default, user must disable it Drop tags on patch 2/6 Jonah Palmer (6): virtio/virtio-pci: Handle extra notification data virtio: Prevent creation of device using notification-data with ioeventfd virtio-mmio: Handle extra notification data virtio-ccw: Handle extra notification data vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition hw/block/vhost-user-blk.c| 1 + hw/net/vhost_net.c | 2 ++ hw/s390x/s390-virtio-ccw.c | 17 +++ hw/scsi/vhost-scsi.c | 1 + hw/scsi/vhost-user-scsi.c| 1 + hw/virtio/vhost-user-fs.c| 2 +- hw/virtio/vhost-user-vsock.c | 1 + hw/virtio/virtio-mmio.c | 10 +++-- hw/virtio/virtio-pci.c | 11 +++--- hw/virtio/virtio.c | 40 include/hw/virtio/virtio.h | 6 +- net/vhost-vdpa.c | 1 + 12 files changed, 82 insertions(+), 11 deletions(-) -- 2.39.3
[PATCH v3 for 9.1 4/6] virtio-ccw: Handle extra notification data
Add support to virtio-ccw devices for handling the extra data sent from
the driver to the device when the VIRTIO_F_NOTIFICATION_DATA transport
feature has been negotiated.
The extra data that's passed to the virtio-ccw device when this feature
is enabled varies depending on the device's virtqueue layout.
That data passed to the virtio-ccw device is in the same format as the
data passed to virtio-pci devices.
Signed-off-by: Jonah Palmer
---
hw/s390x/s390-virtio-ccw.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b1dcb3857f..b550adfc68 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -140,9 +140,11 @@ static void subsystem_reset(void)
static int virtio_ccw_hcall_notify(const uint64_t *args)
{
uint64_t subch_id = args[0];
-uint64_t queue = args[1];
+uint64_t data = args[1];
SubchDev *sch;
+VirtIODevice *vdev;
int cssid, ssid, schid, m;
+uint16_t vq_idx = data;
if (ioinst_disassemble_sch_ident(subch_id, , , , )) {
return -EINVAL;
@@ -151,12 +153,19 @@ static int virtio_ccw_hcall_notify(const uint64_t *args)
if (!sch || !css_subch_visible(sch)) {
return -EINVAL;
}
-if (queue >= VIRTIO_QUEUE_MAX) {
+
+vdev = virtio_ccw_get_vdev(sch);
+if (vq_idx >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, vq_idx)) {
return -EINVAL;
}
-virtio_queue_notify(virtio_ccw_get_vdev(sch), queue);
-return 0;
+if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev, vq_idx),
+ (data >> 16) & 0x);
+}
+
+virtio_queue_notify(vdev, vq_idx);
+return 0;
}
static int virtio_ccw_hcall_early_printk(const uint64_t *args)
--
2.39.3
[PATCH v3 for 9.1 1/6] virtio/virtio-pci: Handle extra notification data
Add support to virtio-pci devices for handling the extra data sent
from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
transport feature has been negotiated.
The extra data that's passed to the virtio-pci device when this
feature is enabled varies depending on the device's virtqueue
layout.
In a split virtqueue layout, this data includes:
- upper 16 bits: shadow_avail_idx
- lower 16 bits: virtqueue index
In a packed virtqueue layout, this data includes:
- upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
- lower 16 bits: virtqueue index
Signed-off-by: Jonah Palmer
---
hw/virtio/virtio-pci.c | 11 ---
hw/virtio/virtio.c | 18 ++
include/hw/virtio/virtio.h | 2 ++
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index cb6940fc0e..f3e0a08f53 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t
addr, uint32_t val)
{
VirtIOPCIProxy *proxy = opaque;
VirtIODevice *vdev = virtio_bus_get_device(>bus);
-uint16_t vector;
+uint16_t vector, vq_idx;
hwaddr pa;
switch (addr) {
@@ -408,8 +408,13 @@ static void virtio_ioport_write(void *opaque, uint32_t
addr, uint32_t val)
vdev->queue_sel = val;
break;
case VIRTIO_PCI_QUEUE_NOTIFY:
-if (val < VIRTIO_QUEUE_MAX) {
-virtio_queue_notify(vdev, val);
+vq_idx = val;
+if (vq_idx < VIRTIO_QUEUE_MAX && virtio_queue_get_num(vdev, vq_idx)) {
+if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev,
vq_idx),
+ val >> 16);
+}
+virtio_queue_notify(vdev, vq_idx);
}
break;
case VIRTIO_PCI_STATUS:
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d229755eae..463426ca92 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n,
int align)
}
}
+void virtio_queue_set_shadow_avail_idx(VirtQueue *vq, uint16_t
shadow_avail_idx)
+{
+if (!vq->vring.desc) {
+return;
+}
+
+/*
+ * 16-bit data for packed VQs include 1-bit wrap counter and
+ * 15-bit shadow_avail_idx.
+ */
+if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+vq->shadow_avail_wrap_counter = (shadow_avail_idx >> 15) & 0x1;
+vq->shadow_avail_idx = shadow_avail_idx & 0x7FFF;
+} else {
+vq->shadow_avail_idx = shadow_avail_idx;
+}
+}
+
static void virtio_queue_notify_vq(VirtQueue *vq)
{
if (vq->vring.desc && vq->handle_output) {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c8f72850bc..cdd4f86b61 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -306,6 +306,8 @@ int virtio_queue_ready(VirtQueue *vq);
int virtio_queue_empty(VirtQueue *vq);
+void virtio_queue_set_shadow_avail_idx(VirtQueue *vq, uint16_t idx);
+
/* Host binding interface. */
uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr);
--
2.39.3
[PATCH v3 for 9.1 2/6] virtio: Prevent creation of device using notification-data with ioeventfd
Prevent the realization of a virtio device that attempts to use the
VIRTIO_F_NOTIFICATION_DATA transport feature without disabling
ioeventfd.
Due to ioeventfd not being able to carry the extra data associated with
this feature, having both enabled is a functional mismatch and therefore
Qemu should not continue the device's realization process.
Although the device does not yet know if the feature will be
successfully negotiated, many devices using this feature wont actually
work without this extra data and would fail FEATURES_OK anyway.
If ioeventfd is able to work with the extra notification data in the
future, this compatibility check can be removed.
Signed-off-by: Jonah Palmer
---
hw/virtio/virtio.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 463426ca92..f9cb8d1e5c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2971,6 +2971,20 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
return ret;
}
+static void virtio_device_check_notification_compatibility(VirtIODevice *vdev,
+ Error **errp)
+{
+VirtioBusState *bus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
+VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
+DeviceState *proxy = DEVICE(BUS(bus)->parent);
+
+if (virtio_host_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) &&
+k->ioeventfd_enabled(proxy)) {
+error_setg(errp,
+ "notification_data=on without ioeventfd=off is not
supported");
+}
+}
+
size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
uint64_t host_features)
{
@@ -3731,6 +3745,14 @@ static void virtio_device_realize(DeviceState *dev,
Error **errp)
}
}
+/* Devices should not use both ioeventfd and notification data feature */
+virtio_device_check_notification_compatibility(vdev, );
+if (err != NULL) {
+error_propagate(errp, err);
+vdc->unrealize(dev);
+return;
+}
+
virtio_bus_device_plugged(vdev, );
if (err != NULL) {
error_propagate(errp, err);
--
2.39.3
[PATCH v3 for 9.1 3/6] virtio-mmio: Handle extra notification data
Add support to virtio-mmio devices for handling the extra data sent from
the driver to the device when the VIRTIO_F_NOTIFICATION_DATA transport
feature has been negotiated.
The extra data that's passed to the virtio-mmio device when this feature
is enabled varies depending on the device's virtqueue layout.
The data passed to the virtio-mmio device is in the same format as the
data passed to virtio-pci devices.
Signed-off-by: Jonah Palmer
---
hw/virtio/virtio-mmio.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 22f9fbcf5a..003c363f0b 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -248,6 +248,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset,
uint64_t value,
{
VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
VirtIODevice *vdev = virtio_bus_get_device(>bus);
+uint16_t vq_idx;
trace_virtio_mmio_write_offset(offset, value);
@@ -407,8 +408,13 @@ static void virtio_mmio_write(void *opaque, hwaddr offset,
uint64_t value,
}
break;
case VIRTIO_MMIO_QUEUE_NOTIFY:
-if (value < VIRTIO_QUEUE_MAX) {
-virtio_queue_notify(vdev, value);
+vq_idx = value;
+if (vq_idx < VIRTIO_QUEUE_MAX && virtio_queue_get_num(vdev, vq_idx)) {
+if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev,
vq_idx),
+ (value >> 16) & 0x);
+}
+virtio_queue_notify(vdev, vq_idx);
}
break;
case VIRTIO_MMIO_INTERRUPT_ACK:
--
2.39.3
Re: [PATCH] scsi-generic: fix io_timeout property not applying
Lorenz Brun writes:
> The io_timeout property, introduced in c9b6609 (part of 6.0) is
> silently overwritten by the hardcoded default value of 30 seconds
> (DEFAULT_IO_TIMEOUT) in scsi_generic_realize because that function is
> being called after the properties have already been applied.
>
> The property definition already has a default value which is applied
> correctly when no value is explicitly set, so we can just remove the
> code which overrides the io_timeout completely.
>
> This has been tested by stracing SG_IO operations with the io_timeout
> property set and unset and now sets the timeout field in the ioctl
> request to the proper value.
>
> Fixes: c9b6609b69facad ("scsi: make io_timeout configurable")
> Signed-off-by: Lorenz Brun
> ---
> hw/scsi/scsi-generic.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index b7b04e1d63..ee945f87e3 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -752,7 +752,6 @@ static void scsi_generic_realize(SCSIDevice *s, Error
> **errp)
>
> /* Only used by scsi-block, but initialize it nevertheless to be clean.
> */
> s->default_scsi_version = -1;
> -s->io_timeout = DEFAULT_IO_TIMEOUT;
> scsi_generic_read_device_inquiry(s);
> }
Reviewed-by: Alex Bennée
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists
On 15/3/24 16:23, Markus Armbruster wrote: Entities with names starting with q_obj_ are implicit object types. Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity() can only return a QAPISchemaObjectType. Assert that. Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH] scsi-generic: fix io_timeout property not applying
The io_timeout property, introduced in c9b6609 (part of 6.0) is
silently overwritten by the hardcoded default value of 30 seconds
(DEFAULT_IO_TIMEOUT) in scsi_generic_realize because that function is
being called after the properties have already been applied.
The property definition already has a default value which is applied
correctly when no value is explicitly set, so we can just remove the
code which overrides the io_timeout completely.
This has been tested by stracing SG_IO operations with the io_timeout
property set and unset and now sets the timeout field in the ioctl
request to the proper value.
Fixes: c9b6609b69facad ("scsi: make io_timeout configurable")
Signed-off-by: Lorenz Brun
---
hw/scsi/scsi-generic.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index b7b04e1d63..ee945f87e3 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -752,7 +752,6 @@ static void scsi_generic_realize(SCSIDevice *s, Error
**errp)
/* Only used by scsi-block, but initialize it nevertheless to be clean. */
s->default_scsi_version = -1;
-s->io_timeout = DEFAULT_IO_TIMEOUT;
scsi_generic_read_device_inquiry(s);
}
--
2.42.0
Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs
[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)? What about the old getfd? Is it obsolete because it relies on monitor object? Or maybe it's still in use? It would be greatly helpful if there can be a summary of how libvirt uses fd for migration purpose. Thanks, -- Peter Xu
[PATCH for-9.0 v3] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
VDUSE requires that virtqueues are first enabled before the DRIVER_OK
status flag is set; with the current API of the kernel module, it is
impossible to enable the opposite order in our block export code because
userspace is not notified when a virtqueue is enabled.
This requirement also mathces the normal initialisation order as done by
the generic vhost code in QEMU. However, commit 6c482547 accidentally
changed the order for vdpa-dev and broke access to VDUSE devices with
this.
This changes vdpa-dev to use the normal order again and use the standard
vhost callback .vhost_set_vring_enable for this. VDUSE devices can be
used with vdpa-dev again after this fix.
vhost_net intentionally avoided enabling the vrings for vdpa and does
this manually later while it does enable them for other vhost backends.
Reflect this in the vhost_net code and return early for vdpa, so that
the behaviour doesn't change for this device.
Cc: qemu-sta...@nongnu.org
Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
Signed-off-by: Kevin Wolf
---
v2:
- Actually make use of the @enable parameter
- Change vhost_net to preserve the current behaviour
v3:
- Updated trace point [Stefano]
- Fixed typo in comment [Stefano]
hw/net/vhost_net.c | 10 ++
hw/virtio/vdpa-dev.c | 5 +
hw/virtio/vhost-vdpa.c | 29 ++---
hw/virtio/vhost.c | 8 +++-
hw/virtio/trace-events | 2 +-
5 files changed, 45 insertions(+), 9 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e8e1661646..fd1a93701a 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable)
VHostNetState *net = get_vhost_net(nc);
const VhostOps *vhost_ops = net->dev.vhost_ops;
+/*
+ * vhost-vdpa network devices need to enable dataplane virtqueues after
+ * DRIVER_OK, so they can recover device state before starting dataplane.
+ * Because of that, we don't enable virtqueues here and leave it to
+ * net/vhost-vdpa.c.
+ */
+if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+return 0;
+}
+
nc->vring_enable = enable;
if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index eb9ecea83b..13e87f06f6 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev,
Error **errp)
s->dev.acked_features = vdev->guest_features;
-ret = vhost_dev_start(>dev, vdev, false);
+ret = vhost_dev_start(>dev, vdev, true);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error starting vhost");
goto err_guest_notifiers;
}
-for (i = 0; i < s->dev.nvqs; ++i) {
-vhost_vdpa_set_vring_ready(>vdpa, i);
-}
s->started = true;
/*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ddae494ca8..31453466af 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -886,19 +886,41 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev,
int idx)
return idx;
}
-int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned idx,
+ int enable)
{
struct vhost_dev *dev = v->dev;
struct vhost_vring_state state = {
.index = idx,
-.num = 1,
+.num = enable,
};
int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, );
-trace_vhost_vdpa_set_vring_ready(dev, idx, r);
+trace_vhost_vdpa_set_vring_enable_one(dev, idx, enable, r);
return r;
}
+static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable)
+{
+struct vhost_vdpa *v = dev->opaque;
+unsigned int i;
+int ret;
+
+for (i = 0; i < dev->nvqs; ++i) {
+ret = vhost_vdpa_set_vring_enable_one(v, i, enable);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
+
+int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+{
+return vhost_vdpa_set_vring_enable_one(v, idx, 1);
+}
+
static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
int fd)
{
@@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = {
.vhost_set_features = vhost_vdpa_set_features,
.vhost_reset_device = vhost_vdpa_reset_device,
.vhost_get_vq_index = vhost_vdpa_get_vq_index,
+.vhost_set_vring_enable = vhost_vdpa_set_vring_enable,
.vhost_get_config = vhost_vdpa_get_config,
.vhost_set_config = vhost_vdpa_set_config,
.vhost_requires_shm_log = NULL,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..a66186 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1984,7 +1984,13 @@ static int vhost_dev_set_vring_enable(struct vhost_dev
*hdev, int enable)
return
Re: [PATCH for-9.0 v2] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
Am 15.03.2024 um 16:07 hat Stefano Garzarella geschrieben:
> On Fri, Mar 15, 2024 at 03:03:31PM +0100, Kevin Wolf wrote:
> > VDUSE requires that virtqueues are first enabled before the DRIVER_OK
> > status flag is set; with the current API of the kernel module, it is
> > impossible to enable the opposite order in our block export code because
> > userspace is not notified when a virtqueue is enabled.
> >
> > This requirement also mathces the normal initialisation order as done by
> > the generic vhost code in QEMU. However, commit 6c482547 accidentally
> > changed the order for vdpa-dev and broke access to VDUSE devices with
> > this.
> >
> > This changes vdpa-dev to use the normal order again and use the standard
> > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be
> > used with vdpa-dev again after this fix.
> >
> > vhost_net intentionally avoided enabling the vrings for vdpa and does
> > this manually later while it does enable them for other vhost backends.
> > Reflect this in the vhost_net code and return early for vdpa, so that
> > the behaviour doesn't change for this device.
> >
> > Cc: qemu-sta...@nongnu.org
> > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> > Signed-off-by: Kevin Wolf
> > ---
> > v2:
> > - Actually make use of the @enable parameter
> > - Change vhost_net to preserve the current behaviour
> >
> > hw/net/vhost_net.c | 10 ++
> > hw/virtio/vdpa-dev.c | 5 +
> > hw/virtio/vhost-vdpa.c | 27 +--
> > hw/virtio/vhost.c | 8 +++-
> > 4 files changed, 43 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index e8e1661646..fd1a93701a 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int
> > enable)
> > VHostNetState *net = get_vhost_net(nc);
> > const VhostOps *vhost_ops = net->dev.vhost_ops;
> >
> > +/*
> > + * vhost-vdpa network devices need to enable dataplane virtqueues after
> > + * DRIVER_OK, so they can recover device state before starting
> > dataplane.
> > + * Because of that, we don't enable virtqueues here and leave it to
> > + * net/vhost-vdpa.c.
> > + */
> > +if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > +return 0;
> > +}
> > +
> > nc->vring_enable = enable;
> >
> > if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
> > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> > index eb9ecea83b..13e87f06f6 100644
> > --- a/hw/virtio/vdpa-dev.c
> > +++ b/hw/virtio/vdpa-dev.c
> > @@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice
> > *vdev, Error **errp)
> >
> > s->dev.acked_features = vdev->guest_features;
> >
> > -ret = vhost_dev_start(>dev, vdev, false);
> > +ret = vhost_dev_start(>dev, vdev, true);
> > if (ret < 0) {
> > error_setg_errno(errp, -ret, "Error starting vhost");
> > goto err_guest_notifiers;
> > }
> > -for (i = 0; i < s->dev.nvqs; ++i) {
> > -vhost_vdpa_set_vring_ready(>vdpa, i);
> > -}
> > s->started = true;
> >
> > /*
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index ddae494ca8..401afac2f5 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -886,12 +886,13 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev
> > *dev, int idx)
> > return idx;
> > }
> >
> > -int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
> > +static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned
> > idx,
> > + int enable)
> > {
> > struct vhost_dev *dev = v->dev;
> > struct vhost_vring_state state = {
> > .index = idx,
> > -.num = 1,
> > +.num = enable,
> > };
> > int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, );
> >
>
> After this line we now have:
>
> trace_vhost_vdpa_set_vring_ready(dev, idx, r);
>
> Should we rename it or move it to the new function?
>
> If we rename it, we should trace also `enable`.
I think renaming is better so that we cover all code paths that enable a
vring. I'll change this and send a v3.
> > @@ -899,6 +900,27 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa
> > *v, unsigned idx)
> > return r;
> > }
> >
> > +static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable)
> > +{
> > +struct vhost_vdpa *v = dev->opaque;
> > +unsigned int i;
> > +int ret;
> > +
> > +for (i = 0; i < dev->nvqs; ++i) {
> > +ret = vhost_vdpa_set_vring_enable_one(v, i, enable);
> > +if (ret < 0) {
> > +return ret;
> > +}
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
> > +{
> > +return vhost_vdpa_set_vring_enable_one(v, idx, 1);
> > +}
> > +
> > static int
[PATCH v5 11/25] qapi/schema: assert resolve_type has 'info' and 'what' args on error
From: John Snow resolve_type() is generally used to resolve configuration-provided type names into type objects, and generally requires valid 'info' and 'what' parameters. In some cases, such as with QAPISchemaArrayType.check(), resolve_type may be used to resolve built-in types and as such will not have an 'info' argument, but also must not fail in this scenario. Use an assertion to sate mypy that we will indeed have 'info' and 'what' parameters for the error pathway in resolve_type. Note: there are only three callsites to resolve_type at present where "info" is perceived by mypy to be possibly None: 1) QAPISchemaArrayType.check() 2) QAPISchemaObjectTypeMember.check() 3) QAPISchemaEvent.check() Of those three, only the first actually ever passes None; the other two are limited by their base class initializers which accept info=None, but neither subclass actually use a None value in practice, currently. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 1034825415..0ef9b3398a 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -996,6 +996,7 @@ def lookup_type(self, name): def resolve_type(self, name, info, what): typ = self.lookup_type(name) if not typ: +assert info and what # built-in types must not fail lookup if callable(what): what = what(info) raise QAPISemError( -- 2.44.0
[PATCH v5 05/25] qapi: create QAPISchemaDefinition
From: John Snow
Include entities don't have names, but we generally expect "entities" to
have names. Reclassify all entities with names as *definitions*, leaving
the nameless include entities as QAPISchemaEntity instances.
This is primarily to help simplify typing around expectations of what
callers expect for properties of an "entity".
Suggested-by: Markus Armbruster
Signed-off-by: John Snow
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
scripts/qapi/schema.py | 144 +++--
1 file changed, 82 insertions(+), 62 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 117f0f78f0..b298c8b4f9 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -55,14 +55,13 @@ def is_present(self):
class QAPISchemaEntity:
-meta: Optional[str] = None
+"""
+A schema entity.
-def __init__(self, name: str, info, doc, ifcond=None, features=None):
-assert name is None or isinstance(name, str)
-for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
-f.set_defined_in(name)
-self.name = name
+This is either a directive, such as include, or a definition.
+The latter uses sub-class `QAPISchemaDefinition`.
+"""
+def __init__(self, info):
self._module = None
# For explicitly defined entities, info points to the (explicit)
# definition. For builtins (and their arrays), info is None.
@@ -70,33 +69,17 @@ def __init__(self, name: str, info, doc, ifcond=None,
features=None):
# triggered the implicit definition (there may be more than one
# such place).
self.info = info
-self.doc = doc
-self._ifcond = ifcond or QAPISchemaIfCond()
-self.features = features or []
self._checked = False
def __repr__(self):
-if self.name is None:
-return "<%s at 0x%x>" % (type(self).__name__, id(self))
-return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
-id(self))
-
-def c_name(self):
-return c_name(self.name)
+return "<%s at 0x%x>" % (type(self).__name__, id(self))
def check(self, schema):
# pylint: disable=unused-argument
-assert not self._checked
-seen = {}
-for f in self.features:
-f.check_clash(self.info, seen)
self._checked = True
def connect_doc(self, doc=None):
-doc = doc or self.doc
-if doc:
-for f in self.features:
-doc.connect_feature(f)
+pass
def _set_module(self, schema, info):
assert self._checked
@@ -107,6 +90,46 @@ def _set_module(self, schema, info):
def set_module(self, schema):
self._set_module(schema, self.info)
+def visit(self, visitor):
+# pylint: disable=unused-argument
+assert self._checked
+
+
+class QAPISchemaDefinition(QAPISchemaEntity):
+meta: Optional[str] = None
+
+def __init__(self, name: str, info, doc, ifcond=None, features=None):
+assert isinstance(name, str)
+super().__init__(info)
+for f in features or []:
+assert isinstance(f, QAPISchemaFeature)
+f.set_defined_in(name)
+self.name = name
+self.doc = doc
+self._ifcond = ifcond or QAPISchemaIfCond()
+self.features = features or []
+
+def __repr__(self):
+return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
+id(self))
+
+def c_name(self):
+return c_name(self.name)
+
+def check(self, schema):
+assert not self._checked
+super().check(schema)
+seen = {}
+for f in self.features:
+f.check_clash(self.info, seen)
+
+def connect_doc(self, doc=None):
+super().connect_doc(doc)
+doc = doc or self.doc
+if doc:
+for f in self.features:
+doc.connect_feature(f)
+
@property
def ifcond(self):
assert self._checked
@@ -115,10 +138,6 @@ def ifcond(self):
def is_implicit(self):
return not self.info
-def visit(self, visitor):
-# pylint: disable=unused-argument
-assert self._checked
-
def describe(self):
assert self.meta
return "%s '%s'" % (self.meta, self.name)
@@ -218,7 +237,7 @@ def visit(self, visitor):
class QAPISchemaInclude(QAPISchemaEntity):
def __init__(self, sub_module, info):
-super().__init__(None, info, None)
+super().__init__(info)
self._sub_module = sub_module
def visit(self, visitor):
@@ -226,7 +245,7 @@ def visit(self, visitor):
visitor.visit_include(self._sub_module.name, self.info)
-class QAPISchemaType(QAPISchemaEntity):
+class QAPISchemaType(QAPISchemaDefinition):
# Return the C type for common use.
# For the types we commonly box, this is
[PATCH v5 23/25] qapi/schema: remove unnecessary asserts
From: John Snow
With strict typing enabled, these runtime statements aren't necessary
anymore; we can prove them statically.
Signed-off-by: John Snow
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
scripts/qapi/schema.py | 25 -
1 file changed, 25 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index ef4d6a7def..e52930a48a 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -123,10 +123,8 @@ def __init__(
ifcond: Optional[QAPISchemaIfCond] = None,
features: Optional[List[QAPISchemaFeature]] = None,
):
-assert isinstance(name, str)
super().__init__(info)
for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
f.set_defined_in(name)
self.name = name
self.doc = doc
@@ -163,7 +161,6 @@ def is_implicit(self) -> bool:
return not self.info
def describe(self) -> str:
-assert self.meta
return "%s '%s'" % (self.meta, self.name)
@@ -377,7 +374,6 @@ def check(self, schema: QAPISchema) -> None:
f"feature '{feat.name}' is not supported for types")
def describe(self) -> str:
-assert self.meta
return "%s type '%s'" % (self.meta, self.name)
@@ -386,7 +382,6 @@ class QAPISchemaBuiltinType(QAPISchemaType):
def __init__(self, name: str, json_type: str, c_type: str):
super().__init__(name, None, None)
-assert not c_type or isinstance(c_type, str)
assert json_type in ('string', 'number', 'int', 'boolean', 'null',
'value')
self._json_type_name = json_type
@@ -429,9 +424,7 @@ def __init__(
):
super().__init__(name, info, doc, ifcond, features)
for m in members:
-assert isinstance(m, QAPISchemaEnumMember)
m.set_defined_in(name)
-assert prefix is None or isinstance(prefix, str)
self.members = members
self.prefix = prefix
@@ -474,7 +467,6 @@ def __init__(
self, name: str, info: Optional[QAPISourceInfo], element_type: str
):
super().__init__(name, info, None)
-assert isinstance(element_type, str)
self._element_type_name = element_type
self.element_type: QAPISchemaType
@@ -519,7 +511,6 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
self.element_type)
def describe(self) -> str:
-assert self.meta
return "%s type ['%s']" % (self.meta, self._element_type_name)
@@ -539,12 +530,9 @@ def __init__(
# union has base, variants, and no local_members
super().__init__(name, info, doc, ifcond, features)
self.meta = 'union' if variants else 'struct'
-assert base is None or isinstance(base, str)
for m in local_members:
-assert isinstance(m, QAPISchemaObjectTypeMember)
m.set_defined_in(name)
if variants is not None:
-assert isinstance(variants, QAPISchemaVariants)
variants.set_defined_in(name)
self._base_name = base
self.base = None
@@ -666,7 +654,6 @@ def __init__(
variants: QAPISchemaVariants,
):
super().__init__(name, info, doc, ifcond, features)
-assert isinstance(variants, QAPISchemaVariants)
assert variants.tag_member
variants.set_defined_in(name)
variants.tag_member.set_defined_in(self.name)
@@ -742,8 +729,6 @@ def __init__(
assert bool(tag_member) != bool(tag_name)
assert (isinstance(tag_name, str) or
isinstance(tag_member, QAPISchemaObjectTypeMember))
-for v in variants:
-assert isinstance(v, QAPISchemaVariant)
self._tag_name = tag_name
self.info = info
self._tag_member = tag_member
@@ -858,7 +843,6 @@ def __init__(
info: Optional[QAPISourceInfo],
ifcond: Optional[QAPISchemaIfCond] = None,
):
-assert isinstance(name, str)
self.name = name
self.info = info
self.ifcond = ifcond or QAPISchemaIfCond()
@@ -926,7 +910,6 @@ def __init__(
):
super().__init__(name, info, ifcond)
for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
f.set_defined_in(name)
self.features = features or []
@@ -955,10 +938,7 @@ def __init__(
features: Optional[List[QAPISchemaFeature]] = None,
):
super().__init__(name, info, ifcond)
-assert isinstance(typ, str)
-assert isinstance(optional, bool)
for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
f.set_defined_in(name)
self._type_name = typ
self.type: QAPISchemaType # set during check()
@@ -966,7 +946,6 @@ def __init__(
self.features = features or []
def need_has(self) -> bool:
-
[PATCH v5 20/25] qapi/parser.py: assert member.info is present in connect_member
From: John Snow Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 2f3c704fa2..7b13a583ac 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -707,6 +707,7 @@ def append_line(self, line: str) -> None: def connect_member(self, member: 'QAPISchemaMember') -> None: if member.name not in self.args: +assert member.info if self.symbol not in member.info.pragma.documentation_exceptions: raise QAPISemError(member.info, "%s '%s' lacks documentation" -- 2.44.0
[PATCH v5 17/25] qapi/schema: fix typing for QAPISchemaVariants.tag_member
From: John Snow
There are two related changes here:
(1) We need to perform type narrowing for resolving the type of
tag_member during check(), and
(2) tag_member is a delayed initialization field, but we can hide it
behind a property that raises an Exception if it's called too
early. This simplifies the typing in quite a few places and avoids
needing to assert that the "tag_member is not None" at a dozen
callsites, which can be confusing and suggest the wrong thing to a
drive-by contributor.
Signed-off-by: John Snow
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
scripts/qapi/schema.py | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 74b0d7b007..9c138badb0 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -627,25 +627,39 @@ def __init__(self, tag_name, info, tag_member, variants):
assert isinstance(v, QAPISchemaVariant)
self._tag_name = tag_name
self.info = info
-self.tag_member = tag_member
+self._tag_member: Optional[QAPISchemaObjectTypeMember] = tag_member
self.variants = variants
+@property
+def tag_member(self) -> 'QAPISchemaObjectTypeMember':
+if self._tag_member is None:
+raise RuntimeError(
+"QAPISchemaVariants has no tag_member property until "
+"after check() has been run."
+)
+return self._tag_member
+
def set_defined_in(self, name):
for v in self.variants:
v.set_defined_in(name)
def check(self, schema, seen):
if self._tag_name: # union
-self.tag_member = seen.get(c_name(self._tag_name))
+# We need to narrow the member type:
+tmp = seen.get(c_name(self._tag_name))
+assert tmp is None or isinstance(tmp, QAPISchemaObjectTypeMember)
+self._tag_member = tmp
+
base = "'base'"
# Pointing to the base type when not implicit would be
# nice, but we don't know it here
-if not self.tag_member or self._tag_name != self.tag_member.name:
+if not self._tag_member or self._tag_name != self._tag_member.name:
raise QAPISemError(
self.info,
"discriminator '%s' is not a member of %s"
% (self._tag_name, base))
# Here we do:
+assert self.tag_member.defined_in
base_type = schema.lookup_type(self.tag_member.defined_in)
assert base_type
if not base_type.is_implicit():
@@ -666,11 +680,13 @@ def check(self, schema, seen):
"discriminator member '%s' of %s must not be conditional"
% (self._tag_name, base))
else: # alternate
+assert self._tag_member
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
assert not self.tag_member.optional
assert not self.tag_member.ifcond.is_present()
if self._tag_name: # union
# branches that are not explicitly covered get an empty type
+assert self.tag_member.defined_in
cases = {v.name for v in self.variants}
for m in self.tag_member.type.members:
if m.name not in cases:
--
2.44.0
[PATCH v5 22/25] qapi/schema: turn on mypy strictness
From: John Snow This patch can be rolled in with the previous one once the series is ready for merge, but for work-in-progress' sake, it's separate here. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/mypy.ini | 5 - 1 file changed, 5 deletions(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 56e0dfb132..8109470a03 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -2,8 +2,3 @@ strict = True disallow_untyped_calls = False python_version = 3.8 - -[mypy-qapi.schema] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False -- 2.44.0
[PATCH v5 08/25] qapi/schema: make c_type() and json_type() abstract methods
From: John Snow These methods should always return a str, it's only the default abstract implementation that doesn't. They can be marked "abstract", which requires subclasses to override the method with the proper return type. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 48f157fb91..0b01c841ff 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -16,6 +16,7 @@ # TODO catching name collisions in generated code would be nice +from abc import ABC, abstractmethod from collections import OrderedDict import os import re @@ -245,9 +246,10 @@ def visit(self, visitor): visitor.visit_include(self._sub_module.name, self.info) -class QAPISchemaType(QAPISchemaDefinition): +class QAPISchemaType(QAPISchemaDefinition, ABC): # Return the C type for common use. # For the types we commonly box, this is a pointer type. +@abstractmethod def c_type(self): pass @@ -259,6 +261,7 @@ def c_param_type(self): def c_unboxed_type(self): return self.c_type() +@abstractmethod def json_type(self): pass -- 2.44.0
[PATCH v5 03/25] qapi: sort pylint suppressions
From: John Snow Suggested-by: Markus Armbruster Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/pylintrc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index 90546df534..1342412c3c 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -16,13 +16,13 @@ ignore-patterns=schema.py, # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use "--disable=all --enable=classes # --disable=W". -disable=fixme, +disable=consider-using-f-string, +fixme, missing-docstring, too-many-arguments, too-many-branches, -too-many-statements, too-many-instance-attributes, -consider-using-f-string, +too-many-statements, useless-option-value, [REPORTS] -- 2.44.0
[PATCH v5 21/25] qapi/schema: add type hints
From: John Snow
This patch only adds type hints, which aren't utilized at runtime and
don't change the behavior of this module in any way.
In a scant few locations, type hints are removed where no longer
necessary due to inference power from typing all of the rest of
creation; and any type hints that no longer need string quotes are
changed.
Signed-off-by: John Snow
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
scripts/qapi/schema.py | 568 -
1 file changed, 396 insertions(+), 172 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 177bfa0d11..ef4d6a7def 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -16,11 +16,21 @@
# TODO catching name collisions in generated code would be nice
+from __future__ import annotations
+
from abc import ABC, abstractmethod
from collections import OrderedDict
import os
import re
-from typing import List, Optional, cast
+from typing import (
+Any,
+Callable,
+Dict,
+List,
+Optional,
+Union,
+cast,
+)
from .common import (
POINTER_SUFFIX,
@@ -32,26 +42,30 @@
)
from .error import QAPIError, QAPISemError, QAPISourceError
from .expr import check_exprs
-from .parser import QAPIExpression, QAPISchemaParser
+from .parser import QAPIDoc, QAPIExpression, QAPISchemaParser
+from .source import QAPISourceInfo
class QAPISchemaIfCond:
-def __init__(self, ifcond=None):
+def __init__(
+self,
+ifcond: Optional[Union[str, Dict[str, object]]] = None,
+) -> None:
self.ifcond = ifcond
-def _cgen(self):
+def _cgen(self) -> str:
return cgen_ifcond(self.ifcond)
-def gen_if(self):
+def gen_if(self) -> str:
return gen_if(self._cgen())
-def gen_endif(self):
+def gen_endif(self) -> str:
return gen_endif(self._cgen())
-def docgen(self):
+def docgen(self) -> str:
return docgen_ifcond(self.ifcond)
-def is_present(self):
+def is_present(self) -> bool:
return bool(self.ifcond)
@@ -62,8 +76,8 @@ class QAPISchemaEntity:
This is either a directive, such as include, or a definition.
The latter uses sub-class `QAPISchemaDefinition`.
"""
-def __init__(self, info):
-self._module = None
+def __init__(self, info: Optional[QAPISourceInfo]):
+self._module: Optional[QAPISchemaModule] = None
# For explicitly defined entities, info points to the (explicit)
# definition. For builtins (and their arrays), info is None.
# For implicitly defined entities, info points to a place that
@@ -72,34 +86,43 @@ def __init__(self, info):
self.info = info
self._checked = False
-def __repr__(self):
+def __repr__(self) -> str:
return "<%s at 0x%x>" % (type(self).__name__, id(self))
-def check(self, schema):
+def check(self, schema: QAPISchema) -> None:
# pylint: disable=unused-argument
self._checked = True
-def connect_doc(self, doc=None):
+def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
pass
-def _set_module(self, schema, info):
+def _set_module(
+self, schema: QAPISchema, info: Optional[QAPISourceInfo]
+) -> None:
assert self._checked
fname = info.fname if info else QAPISchemaModule.BUILTIN_MODULE_NAME
self._module = schema.module_by_fname(fname)
self._module.add_entity(self)
-def set_module(self, schema):
+def set_module(self, schema: QAPISchema) -> None:
self._set_module(schema, self.info)
-def visit(self, visitor):
+def visit(self, visitor: QAPISchemaVisitor) -> None:
# pylint: disable=unused-argument
assert self._checked
class QAPISchemaDefinition(QAPISchemaEntity):
-meta: Optional[str] = None
+meta: str
-def __init__(self, name: str, info, doc, ifcond=None, features=None):
+def __init__(
+self,
+name: str,
+info: Optional[QAPISourceInfo],
+doc: Optional[QAPIDoc],
+ifcond: Optional[QAPISchemaIfCond] = None,
+features: Optional[List[QAPISchemaFeature]] = None,
+):
assert isinstance(name, str)
super().__init__(info)
for f in features or []:
@@ -110,21 +133,21 @@ def __init__(self, name: str, info, doc, ifcond=None,
features=None):
self._ifcond = ifcond or QAPISchemaIfCond()
self.features = features or []
-def __repr__(self):
+def __repr__(self) -> str:
return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
id(self))
-def c_name(self):
+def c_name(self) -> str:
return c_name(self.name)
-def check(self, schema):
+def check(self, schema: QAPISchema) -> None:
assert not self._checked
super().check(schema)
-seen = {}
+seen: Dict[str,
[PATCH v5 25/25] qapi: Dumb down QAPISchema.lookup_entity()
QAPISchema.lookup_entity() takes an optional type argument, a subtype of QAPISchemaDefinition, and returns that type or None. Callers can use this to save themselves an isinstance() test. The only remaining user of this convenience feature is .lookup_type(). But we don't actually save anything anymore there: we still the isinstance() to help mypy over the hump. Drop the .lookup_entity() argument, and adjust .lookup_type(). Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 18 ++ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index a6180f93c6..5924947fc3 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -1157,20 +1157,14 @@ def _def_definition(self, defn: QAPISchemaDefinition) -> None: defn.info, "%s is already defined" % other_defn.describe()) self._entity_dict[defn.name] = defn -def lookup_entity( -self, -name: str, -typ: Optional[type] = None, -) -> Optional[QAPISchemaDefinition]: -ent = self._entity_dict.get(name) -if typ and not isinstance(ent, typ): -return None -return ent +def lookup_entity(self,name: str) -> Optional[QAPISchemaEntity]: +return self._entity_dict.get(name) def lookup_type(self, name: str) -> Optional[QAPISchemaType]: -typ = self.lookup_entity(name, QAPISchemaType) -assert typ is None or isinstance(typ, QAPISchemaType) -return typ +typ = self.lookup_entity(name) +if isinstance(typ, QAPISchemaType): +return typ +return None def resolve_type( self, -- 2.44.0
[PATCH v5 14/25] qapi/schema: assert info is present when necessary
From: John Snow QAPISchemaInfo arguments can often be None because built-in definitions don't have such information. The type hint can only be Optional[QAPISchemaInfo] then. But, mypy gets upset about all the places where we exploit that it can't actually be None there. Add assertions that will help mypy over the hump, to enable adding type hints in a forthcoming commit. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 087c6e9366..173e27d9e2 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -751,6 +751,7 @@ def describe(self, info): else: assert False +assert info is not None if defined_in != info.defn_name: return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in) return "%s '%s'" % (role, self.name) @@ -841,6 +842,7 @@ def __init__(self, name, info, doc, ifcond, features, self.coroutine = coroutine def check(self, schema): +assert self.info is not None super().check(schema) if self._arg_type_name: arg_type = schema.resolve_type( -- 2.44.0
[PATCH v5 07/25] qapi/schema: declare type for QAPISchemaArrayType.element_type
From: John Snow A QAPISchemaArrayType's element type gets resolved only during .check(). We have QAPISchemaArrayType.__init__() initialize self.element_type = None, and .check() assign the actual type. Using .element_type before .check() is wrong, and hopefully crashes due to the value being None. Works. However, it makes for awkward typing. With .element_type: Optional[QAPISchemaType], mypy is of course unable to see that it's None before .check(), and a QAPISchemaType after. To help it over the hump, we'd have to assert self.element_type is not None before all the (valid) uses. The assertion catches invalid uses, but only at run time; mypy can't flag them. Instead, declare .element_type in .__init__() as QAPISchemaType *without* initializing it. Using .element_type before .check() now certainly crashes, which is an improvement. Mypy still can't flag invalid uses, but that's okay. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 307f8af01a..48f157fb91 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -381,7 +381,7 @@ def __init__(self, name, info, element_type): super().__init__(name, info, None) assert isinstance(element_type, str) self._element_type_name = element_type -self.element_type = None +self.element_type: QAPISchemaType def need_has_if_optional(self): # When FOO is an array, we still need has_FOO to distinguish -- 2.44.0
[PATCH v5 09/25] qapi/schema: adjust type narrowing for mypy's benefit
From: John Snow We already take care to perform some type narrowing for arg_type and ret_type, but not in a way where mypy can utilize the result once we add type hints, e.g.: qapi/schema.py:833: error: Incompatible types in assignment (expression has type "QAPISchemaType", variable has type "Optional[QAPISchemaObjectType]") [assignment] qapi/schema.py:893: error: Incompatible types in assignment (expression has type "QAPISchemaType", variable has type "Optional[QAPISchemaObjectType]") [assignment] A simple change to use a temporary variable helps the medicine go down. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 0b01c841ff..e44802369d 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -843,13 +843,14 @@ def __init__(self, name, info, doc, ifcond, features, def check(self, schema): super().check(schema) if self._arg_type_name: -self.arg_type = schema.resolve_type( +arg_type = schema.resolve_type( self._arg_type_name, self.info, "command's 'data'") -if not isinstance(self.arg_type, QAPISchemaObjectType): +if not isinstance(arg_type, QAPISchemaObjectType): raise QAPISemError( self.info, "command's 'data' cannot take %s" -% self.arg_type.describe()) +% arg_type.describe()) +self.arg_type = arg_type if self.arg_type.variants and not self.boxed: raise QAPISemError( self.info, @@ -866,8 +867,8 @@ def check(self, schema): if self.name not in self.info.pragma.command_returns_exceptions: typ = self.ret_type if isinstance(typ, QAPISchemaArrayType): -typ = self.ret_type.element_type assert typ +typ = typ.element_type if not isinstance(typ, QAPISchemaObjectType): raise QAPISemError( self.info, @@ -903,13 +904,14 @@ def __init__(self, name, info, doc, ifcond, features, arg_type, boxed): def check(self, schema): super().check(schema) if self._arg_type_name: -self.arg_type = schema.resolve_type( +typ = schema.resolve_type( self._arg_type_name, self.info, "event's 'data'") -if not isinstance(self.arg_type, QAPISchemaObjectType): +if not isinstance(typ, QAPISchemaObjectType): raise QAPISemError( self.info, "event's 'data' cannot take %s" -% self.arg_type.describe()) +% typ.describe()) +self.arg_type = typ if self.arg_type.variants and not self.boxed: raise QAPISemError( self.info, -- 2.44.0
[PATCH v5 13/25] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type
From: John Snow Adjust the expression at the callsite to work around mypy's weak type introspection that believes this expression can resolve to QAPISourceInfo; it cannot. (Fundamentally: self.info only resolves to false in a boolean expression when it is None; therefore this expression may only ever produce Optional[str]. mypy does not know that 'info', when it is a QAPISourceInfo object, cannot ever be false.) Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 0ef9b3398a..087c6e9366 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -395,7 +395,7 @@ def check(self, schema): super().check(schema) self.element_type = schema.resolve_type( self._element_type_name, self.info, -self.info and self.info.defn_meta) +self.info.defn_meta if self.info else None) assert not isinstance(self.element_type, QAPISchemaArrayType) def set_module(self, schema): -- 2.44.0
[PATCH v5 00/25] qapi: statically type schema.py
v5: * PATCH 05: Move QAPISchemaDefinition.check()'s super().check() back to where it was in v3 * PATCH 12: Replaced, necessitating minor adjustments in PATCH 17+22 * PATCH 16: Tweak comment * PATCH 22: Tighten QAPISchema.lookup_entity()'s type hint * PATCH 24+25: New John Snow (22): qapi/parser: fix typo - self.returns.info => self.errors.info qapi/parser: shush up pylint qapi: sort pylint suppressions qapi/schema: add pylint suppressions qapi: create QAPISchemaDefinition qapi/schema: declare type for QAPISchemaObjectTypeMember.type qapi/schema: declare type for QAPISchemaArrayType.element_type qapi/schema: make c_type() and json_type() abstract methods qapi/schema: adjust type narrowing for mypy's benefit qapi/schema: add type narrowing to lookup_type() qapi/schema: assert resolve_type has 'info' and 'what' args on error qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type qapi/schema: assert info is present when necessary qapi/schema: add _check_complete flag qapi/schema: Don't initialize "members" with `None` qapi/schema: fix typing for QAPISchemaVariants.tag_member qapi/schema: assert inner type of QAPISchemaVariants in check_clash() qapi/parser: demote QAPIExpression to Dict[str, Any] qapi/parser.py: assert member.info is present in connect_member qapi/schema: add type hints qapi/schema: turn on mypy strictness qapi/schema: remove unnecessary asserts Markus Armbruster (3): qapi: Assert built-in types exist qapi: Tighten check whether implicit object type already exists qapi: Dumb down QAPISchema.lookup_entity() scripts/qapi/introspect.py | 8 +- scripts/qapi/mypy.ini | 5 - scripts/qapi/parser.py | 7 +- scripts/qapi/pylintrc | 11 +- scripts/qapi/schema.py | 794 - 5 files changed, 537 insertions(+), 288 deletions(-) -- 2.44.0
[PATCH v5 18/25] qapi/schema: assert inner type of QAPISchemaVariants in check_clash()
From: John Snow QAPISchemaVariant's "variants" field is typed as List[QAPISchemaVariant], where the typing for QAPISchemaVariant allows its type field to be any QAPISchemaType. However, QAPISchemaVariant expects that all of its variants contain the narrower QAPISchemaObjectType. This relationship is enforced at runtime in QAPISchemaVariants.check(). This relationship is not embedded in the type system though, so QAPISchemaVariants.check_clash() needs to re-assert this property in order to call QAPISchemaVariant.type.check_clash(). Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 9c138badb0..177bfa0d11 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -716,7 +716,10 @@ def check(self, schema, seen): def check_clash(self, info, seen): for v in self.variants: # Reset seen map for each variant, since qapi names from one -# branch do not affect another branch +# branch do not affect another branch. +# +# v.type's typing is enforced in check() above. +assert isinstance(v.type, QAPISchemaObjectType) v.type.check_clash(info, dict(seen)) -- 2.44.0
[PATCH v5 01/25] qapi/parser: fix typo - self.returns.info => self.errors.info
From: John Snow Small copy-pasto. The correct info field to use in this conditional block is self.errors.info. Fixes: 3a025d3d1ffa Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index d8f76060b8..fed88e9074 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -733,7 +733,7 @@ def check_expr(self, expr: QAPIExpression) -> None: "'Returns' section is only valid for commands") if self.errors: raise QAPISemError( -self.returns.info, +self.errors.info, "'Errors' section is only valid for commands") def check(self) -> None: -- 2.44.0
[PATCH v5 15/25] qapi/schema: add _check_complete flag
From: John Snow Instead of using the None value for the members field, use a dedicated flag to detect recursive misconfigurations. This is intended to assist with subsequent patches that seek to remove the "None" value from the members field (which can never hold that value after the final call to check()) in order to simplify the static typing of that field; avoiding the need of assertions littered at many callsites to eliminate the possibility of the None value. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 173e27d9e2..2c3de72ae6 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -450,12 +450,13 @@ def __init__(self, name, info, doc, ifcond, features, self.local_members = local_members self.variants = variants self.members = None +self._check_complete = False def check(self, schema): # This calls another type T's .check() exactly when the C # struct emitted by gen_object() contains that T's C struct # (pointers don't count). -if self.members is not None: +if self._check_complete: # A previous .check() completed: nothing to do return if self._checked: @@ -464,7 +465,7 @@ def check(self, schema): "object %s contains itself" % self.name) super().check(schema) -assert self._checked and self.members is None +assert self._checked and not self._check_complete seen = OrderedDict() if self._base_name: @@ -487,7 +488,8 @@ def check(self, schema): self.variants.check(schema, seen) self.variants.check_clash(self.info, seen) -self.members = members # mark completed +self.members = members +self._check_complete = True # mark completed # Check that the members of this type do not cause duplicate JSON members, # and update seen to track the members seen so far. Report any errors -- 2.44.0
[PATCH v5 06/25] qapi/schema: declare type for QAPISchemaObjectTypeMember.type
From: John Snow A QAPISchemaObjectTypeMember's type gets resolved only during .check(). We have QAPISchemaObjectTypeMember.__init__() initialize self.type = None, and .check() assign the actual type. Using .type before .check() is wrong, and hopefully crashes due to the value being None. Works. However, it makes for awkward typing. With .type: Optional[QAPISchemaType], mypy is of course unable to see that it's None before .check(), and a QAPISchemaType after. To help it over the hump, we'd have to assert self.type is not None before all the (valid) uses. The assertion catches invalid uses, but only at run time; mypy can't flag them. Instead, declare .type in .__init__() as QAPISchemaType *without* initializing it. Using .type before .check() now certainly crashes, which is an improvement. Mypy still can't flag invalid uses, but that's okay. Addresses typing errors such as these: qapi/schema.py:657: error: "None" has no attribute "alternate_qtype" [attr-defined] qapi/schema.py:662: error: "None" has no attribute "describe" [attr-defined] Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index b298c8b4f9..307f8af01a 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -786,7 +786,7 @@ def __init__(self, name, info, typ, optional, ifcond=None, features=None): assert isinstance(f, QAPISchemaFeature) f.set_defined_in(name) self._type_name = typ -self.type = None +self.type: QAPISchemaType # set during check() self.optional = optional self.features = features or [] -- 2.44.0
[PATCH v5 19/25] qapi/parser: demote QAPIExpression to Dict[str, Any]
From: John Snow Dict[str, object] is a stricter type, but with the way that code is currently arranged, it is infeasible to enforce this strictness. In particular, although expr.py's entire raison d'être is normalization and type-checking of QAPI Expressions, that type information is not "remembered" in any meaningful way by mypy because each individual expression is not downcast to a specific expression type that holds all the details of each expression's unique form. As a result, all of the code in schema.py that deals with actually creating type-safe specialized structures has no guarantee (myopically) that the data it is being passed is correct. There are two ways to solve this: (1) Re-assert that the incoming data is in the shape we expect it to be, or (2) Disable type checking for this data. (1) is appealing to my sense of strictness, but I gotta concede that it is asinine to re-check the shape of a QAPIExpression in schema.py when expr.py has just completed that work at length. The duplication of code and the nightmare thought of needing to update both locations if and when we change the shape of these structures makes me extremely reluctant to go down this route. (2) allows us the chance to miss updating types in the case that types are updated in expr.py, but it *is* an awful lot simpler and, importantly, gets us closer to type checking schema.py *at all*. Something is better than nothing, I'd argue. So, do the simpler dumber thing and worry about future strictness improvements later. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index ec4ebef4e3..2f3c704fa2 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -19,6 +19,7 @@ import re from typing import ( TYPE_CHECKING, +Any, Dict, List, Mapping, @@ -43,7 +44,7 @@ _ExprValue = Union[List[object], Dict[str, object], str, bool] -class QAPIExpression(Dict[str, object]): +class QAPIExpression(Dict[str, Any]): # pylint: disable=too-few-public-methods def __init__(self, data: Mapping[str, object], -- 2.44.0
[PATCH v5 02/25] qapi/parser: shush up pylint
From: John Snow Shhh! Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index fed88e9074..ec4ebef4e3 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -607,6 +607,7 @@ class QAPIDoc: """ class Section: +# pylint: disable=too-few-public-methods def __init__(self, info: QAPISourceInfo, tag: Optional[str] = None): # section source info, i.e. where it begins -- 2.44.0
[PATCH v5 16/25] qapi/schema: Don't initialize "members" with `None`
From: John Snow
Declare, but don't initialize the "members" field with type
List[QAPISchemaObjectTypeMember].
This simplifies the typing from what would otherwise be
Optional[List[T]] to merely List[T]. This removes the need to add
assertions to several callsites that this value is not None - which it
never will be after the delayed initialization in check() anyway.
The type declaration without initialization trick will cause accidental
uses of this field prior to full initialization to raise an
AttributeError.
(Note that it is valid to have an empty members list, see the internal
q_empty object as an example. For this reason, we cannot use the empty
list as a replacement test for full initialization and instead rely on
the _checked/_check_complete fields.)
Signed-off-by: John Snow
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
scripts/qapi/schema.py | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 2c3de72ae6..74b0d7b007 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
from collections import OrderedDict
import os
import re
-from typing import List, Optional
+from typing import List, Optional, cast
from .common import (
POINTER_SUFFIX,
@@ -449,7 +449,7 @@ def __init__(self, name, info, doc, ifcond, features,
self.base = None
self.local_members = local_members
self.variants = variants
-self.members = None
+self.members: List[QAPISchemaObjectTypeMember]
self._check_complete = False
def check(self, schema):
@@ -482,7 +482,11 @@ def check(self, schema):
for m in self.local_members:
m.check(schema)
m.check_clash(self.info, seen)
-members = seen.values()
+
+# self.check_clash() works in terms of the supertype, but
+# self.members is declared List[QAPISchemaObjectTypeMember].
+# Cast down to the subtype.
+members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
if self.variants:
self.variants.check(schema, seen)
@@ -515,11 +519,9 @@ def is_implicit(self):
return self.name.startswith('q_')
def is_empty(self):
-assert self.members is not None
return not self.members and not self.variants
def has_conditional_members(self):
-assert self.members is not None
return any(m.ifcond.is_present() for m in self.members)
def c_name(self):
--
2.44.0
[PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists
Entities with names starting with q_obj_ are implicit object types. Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity() can only return a QAPISchemaObjectType. Assert that. Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index e52930a48a..a6180f93c6 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -1297,8 +1297,9 @@ def _make_implicit_object_type( return None # See also QAPISchemaObjectTypeMember.describe() name = 'q_obj_%s-%s' % (name, role) -typ = self.lookup_entity(name, QAPISchemaObjectType) +typ = self.lookup_entity(name) if typ: +assert(isinstance(typ, QAPISchemaObjectType)) # The implicit object type has multiple users. This can # only be a duplicate definition, which will be flagged # later. -- 2.44.0
[PATCH v5 04/25] qapi/schema: add pylint suppressions
From: John Snow
With this patch, pylint is happy with the file, so enable it in the
configuration.
Signed-off-by: John Snow
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
scripts/qapi/pylintrc | 5 -
scripts/qapi/schema.py | 5 +
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index 1342412c3c..c028a1f9f5 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -1,10 +1,5 @@
[MASTER]
-# Add files or directories matching the regex patterns to the ignore list.
-# The regex matches against base names, not paths.
-ignore-patterns=schema.py,
-
-
[MESSAGES CONTROL]
# Disable the message, report, category or checker with the given id(s). You
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 8ba5665bc6..117f0f78f0 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -12,6 +12,8 @@
# This work is licensed under the terms of the GNU GPL, version 2.
# See the COPYING file in the top-level directory.
+# pylint: disable=too-many-lines
+
# TODO catching name collisions in generated code would be nice
from collections import OrderedDict
@@ -83,6 +85,7 @@ def c_name(self):
return c_name(self.name)
def check(self, schema):
+# pylint: disable=unused-argument
assert not self._checked
seen = {}
for f in self.features:
@@ -113,6 +116,7 @@ def is_implicit(self):
return not self.info
def visit(self, visitor):
+# pylint: disable=unused-argument
assert self._checked
def describe(self):
@@ -131,6 +135,7 @@ def visit_module(self, name):
pass
def visit_needed(self, entity):
+# pylint: disable=unused-argument
# Default to visiting everything
return True
--
2.44.0
[PATCH v5 12/25] qapi: Assert built-in types exist
QAPISchema.lookup_type('FOO') returns a QAPISchemaType when type 'FOO'
exists, else None. It won't return None for built-in types like
'int'.
Since mypy can't see that, it'll complain that we assign the
Optional[QAPISchemaType] returned by .lookup_type() to QAPISchemaType
variables.
Add assertions to help it over the hump.
Signed-off-by: Markus Armbruster
---
scripts/qapi/introspect.py | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae..4679b1bc2c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -227,10 +227,14 @@ def _use_type(self, typ: QAPISchemaType) -> str:
# Map the various integer types to plain int
if typ.json_type() == 'int':
-typ = self._schema.lookup_type('int')
+type_int = self._schema.lookup_type('int')
+assert type_int
+typ = type_int
elif (isinstance(typ, QAPISchemaArrayType) and
typ.element_type.json_type() == 'int'):
-typ = self._schema.lookup_type('intList')
+type_intList = self._schema.lookup_type('intList')
+assert type_intList
+typ = type_intList
# Add type to work queue if new
if typ not in self._used_types:
self._used_types.append(typ)
--
2.44.0
[PATCH v5 10/25] qapi/schema: add type narrowing to lookup_type()
From: John Snow This function is a bit hard to type as-is; mypy needs some assertions to assist with the type narrowing. Signed-off-by: John Snow Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi/schema.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index e44802369d..1034825415 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -989,7 +989,9 @@ def lookup_entity(self, name, typ=None): return ent def lookup_type(self, name): -return self.lookup_entity(name, QAPISchemaType) +typ = self.lookup_entity(name, QAPISchemaType) +assert typ is None or isinstance(typ, QAPISchemaType) +return typ def resolve_type(self, name, info, what): typ = self.lookup_type(name) -- 2.44.0
Re: [PATCH for-9.0 v2] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Fri, Mar 15, 2024 at 03:03:31PM +0100, Kevin Wolf wrote:
VDUSE requires that virtqueues are first enabled before the DRIVER_OK
status flag is set; with the current API of the kernel module, it is
impossible to enable the opposite order in our block export code because
userspace is not notified when a virtqueue is enabled.
This requirement also mathces the normal initialisation order as done by
the generic vhost code in QEMU. However, commit 6c482547 accidentally
changed the order for vdpa-dev and broke access to VDUSE devices with
this.
This changes vdpa-dev to use the normal order again and use the standard
vhost callback .vhost_set_vring_enable for this. VDUSE devices can be
used with vdpa-dev again after this fix.
vhost_net intentionally avoided enabling the vrings for vdpa and does
this manually later while it does enable them for other vhost backends.
Reflect this in the vhost_net code and return early for vdpa, so that
the behaviour doesn't change for this device.
Cc: qemu-sta...@nongnu.org
Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
Signed-off-by: Kevin Wolf
---
v2:
- Actually make use of the @enable parameter
- Change vhost_net to preserve the current behaviour
hw/net/vhost_net.c | 10 ++
hw/virtio/vdpa-dev.c | 5 +
hw/virtio/vhost-vdpa.c | 27 +--
hw/virtio/vhost.c | 8 +++-
4 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e8e1661646..fd1a93701a 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable)
VHostNetState *net = get_vhost_net(nc);
const VhostOps *vhost_ops = net->dev.vhost_ops;
+/*
+ * vhost-vdpa network devices need to enable dataplane virtqueues after
+ * DRIVER_OK, so they can recover device state before starting dataplane.
+ * Because of that, we don't enable virtqueues here and leave it to
+ * net/vhost-vdpa.c.
+ */
+if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+return 0;
+}
+
nc->vring_enable = enable;
if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index eb9ecea83b..13e87f06f6 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev,
Error **errp)
s->dev.acked_features = vdev->guest_features;
-ret = vhost_dev_start(>dev, vdev, false);
+ret = vhost_dev_start(>dev, vdev, true);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error starting vhost");
goto err_guest_notifiers;
}
-for (i = 0; i < s->dev.nvqs; ++i) {
-vhost_vdpa_set_vring_ready(>vdpa, i);
-}
s->started = true;
/*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ddae494ca8..401afac2f5 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -886,12 +886,13 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev,
int idx)
return idx;
}
-int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned idx,
+ int enable)
{
struct vhost_dev *dev = v->dev;
struct vhost_vring_state state = {
.index = idx,
-.num = 1,
+.num = enable,
};
int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, );
After this line we now have:
trace_vhost_vdpa_set_vring_ready(dev, idx, r);
Should we rename it or move it to the new function?
If we rename it, we should trace also `enable`.
@@ -899,6 +900,27 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa
*v, unsigned idx)
return r;
}
+static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable)
+{
+struct vhost_vdpa *v = dev->opaque;
+unsigned int i;
+int ret;
+
+for (i = 0; i < dev->nvqs; ++i) {
+ret = vhost_vdpa_set_vring_enable_one(v, i, enable);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
+
+int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+{
+return vhost_vdpa_set_vring_enable_one(v, idx, 1);
+}
+
static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
int fd)
{
@@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = {
.vhost_set_features = vhost_vdpa_set_features,
.vhost_reset_device = vhost_vdpa_reset_device,
.vhost_get_vq_index = vhost_vdpa_get_vq_index,
+.vhost_set_vring_enable = vhost_vdpa_set_vring_enable,
.vhost_get_config = vhost_vdpa_get_config,
.vhost_set_config = vhost_vdpa_set_config,
.vhost_requires_shm_log = NULL,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..decfb85184 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1984,7 +1984,13 @@ static int
Re: [PATCH v4 22/25] memory: Add Error** argument to memory_get_xlat_addr()
On Wed, Mar 06, 2024 at 02:34:37PM +0100, Cédric Le Goater wrote: > Let the callers do the reporting. This will be useful in > vfio_iommu_map_dirty_notify(). > > Cc: "Michael S. Tsirkin" > Cc: Paolo Bonzini > Cc: David Hildenbrand > Signed-off-by: Cédric Le Goater Reviewed-by: Peter Xu -- Peter Xu
Re: Another CXL/MMIO tcg tlb corner case
> On 15 Mar 2024, at 13.25, Alex Bennée wrote:
>
> Jørgen Hansen writes:
>
>> Hi,
>>
>> While doing some testing using numactl-based interleaving of application
>> memory
>> across regular memory and CXL-based memory using QEMU with tcg, I ran into an
>> issue similar to what we saw a while back - link to old issue:
>> https://lore.kernel.org/qemu-devel/CAFEAcA_a_AyQ=epz3_+cheat8crsk9mou894wbnw_fywamk...@mail.gmail.com/#t.
>>
>> When running:
>>
>> numactl --interleave 0,1 ./cachebench …
>>
>> I hit the following:
>>
>> numactl --interleave 0,1 ./cachebench --json_test_config
>> ../test_configs/hit_ratio/graph_cache_follower_assocs/config.json
>> qemu: fatal: cpu_io_recompile: could not find TB for pc=0x7fffc3926dd4
>
> Ok so this will be because we (by design) don't cache TB's for MMIO
> regions. But as you say:
>>
>> Looking at the tb being executed, it looks like it is a single instruction
>> tb,
>> so with my _very_ limited understanding of tcg, it shouldn’t be necessary to
>> do the IO recompile:
>>
>> (gdb) up 13
>>
>> #13 0x55ca13ac in cpu_loop_exec_tb (tb_exit=0x749ff6d8,
>> last_tb=, pc=, tb=0x7fffc3926cc0
>> , cpu=0x578c19c0) at
>> ../accel/tcg/cpu-exec.c:904
>> 904 tb = cpu_tb_exec(cpu, tb, tb_exit);
>> (gdb) print *tb
>> $1 = {pc = 0, cs_base = 0, flags = 415285939, cflags = 4278321152, size = 7,
>> icount = 1, tc = {ptr = 0x7fffc3926d80 , size =
>> 176}, page_next = {0, 0}, page_addr = {18446744073709551615,
>>18446744073709551615}, jmp_lock = {value = 0}, jmp_reset_offset =
>> {65535, 65535}, jmp_insn_offset = {65535, 65535}, jmp_target_addr =
>> {0, 0}, jmp_list_head = 140736474540928, jmp_list_next = {0, 0},
>> jmp_dest = {0, 0}}
>
> with an icount of 1 we by definition can do io. This is done in the
> translator_loop:
>
>/*
> * Disassemble one instruction. The translate_insn hook should
> * update db->pc_next and db->is_jmp to indicate what should be
> * done next -- either exiting this loop or locate the start of
> * the next instruction.
> */
>if (db->num_insns == db->max_insns) {
>/* Accept I/O on the last instruction. */
>set_can_do_io(db, true);
>}
>
> and we see later on:
>
>tb->icount = db->num_insns;
>
> So I guess we must have gone into the translator loop expecting to
> translate more? (side note having int8_t type for the saved bool value
> seems weird).
>
> Could you confirm by doing something like:
>
> if (tb->icount == 1 && db->max_insns > 1) {
> fprintf(stderr, "ended up doing one insn (%d, %d)", db->max_insns,
> db->saved_can_do_io);
> }
>
> after we set tb->icount?
>
Thanks for looking into this - I tried what you suggest above and it looks
like that is a case that happens quite often (I see 100s of these just when
booting the VM), so it is hard to tell whether it is related directly to the
issue, e.g.,:
ended up doing one insn (512, 0)ended up doing one insn (512, 0)ended up doing
one insn (512, 0)ended up doing one insn (512, 0)ended up doing one insn (512,
0)ended up doing one insn (512, 0)ended up doing one insn (512, 0)qemu: fatal:
cpu_io_recompile: could not find TB for pc=0x7f4264da3d48
RAX= RBX=7f6798e69040 RCX=0205f958
RDX=0205f8f0
RSI= RDI=7ffd5663c720 RBP=7ffd5663c720
RSP=7ffd5663c6c0
R8 =001e R9 =0205f920 R10=0004
R11=7f67984b56b0
R12=7ffd5663c7e0 R13=7f6798e5d0b8 R14=7f6798e5d588
R15=7ffd5663c700
RIP=7f6798e39ffd RFL=0246 [---Z-P-] CPL=3 II=0 A20=1 SMM=0 HLT=0
>> If the application is run entirely out of MMIO memory, things work fine (the
>> previous patches related to this is in Jonathans branch), so one thought is
>> that
>> it is related to having the code on a mix of regular and CXL memory. Since we
>> previously had issues with code crossing page boundaries where only the
>> second
>> page is MMIO, I tried out the following change to the fix introduced for that
>> issue thinking that reverting to the slow path in the middle of the
>> translation
>> might not correctly update can_do_io:
>>
>> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
>> index 38c34009a5..db6ea360e0 100644
>> --- a/accel/tcg/translator.c
>> +++ b/accel/tcg/translator.c
>> @@ -258,6 +258,7 @@ static void *translator_access(CPUArchState *env,
>> DisasContextBase *db,
>> if (unlikely(new_page1 == -1)) {
>> tb_unlock_pages(tb);
>> tb_set_page_addr0(tb, -1);
>> +set_can_do_io(db, true);
>> return NULL;
>> }
>>
>> With that change, things work fine. Not saying that this is a valid fix for
>> the
>> issue, but just trying to provide as much information as possible :)
>
> I can see why this works, my worry is if we address all the early exit
> cases here.
>
>>
>> Thanks,
>> Jorgen
>
> --
>
Re: [PATCH v4 10/25] migration: Add Error** argument to qemu_savevm_state_setup()
On Fri, Mar 15, 2024 at 03:31:28PM +0100, Cédric Le Goater wrote: > On 3/15/24 14:11, Peter Xu wrote: > > On Fri, Mar 15, 2024 at 01:20:49PM +0100, Cédric Le Goater wrote: > > > +static void qemu_savevm_wait_unplug(MigrationState *s, int state) > > > > One more trivial comment: I'd even consider dropping "state" altogether, as > > this should be the only state this function should be invoked. So we can > > perhaps assert it instead of passing it over? > > Yes. If you prefer this implementation I will change. I am fine with either approach, we can wait for 1-2 days to see whether others want to say. Otherwise the other approach actually looks better to me in that it avoids SETUP->UNPLUG->SETUP jumps. And then we wait to see whether UNPLUG can be dropped for either way to go, perhaps starting from adding it into deprecation list if no objections from the relevant folks. Thanks, -- Peter Xu
Re: [PATCH v4 10/25] migration: Add Error** argument to qemu_savevm_state_setup()
On Fri, Mar 15, 2024 at 03:21:27PM +0100, Cédric Le Goater wrote:
> On 3/15/24 13:20, Cédric Le Goater wrote:
> > On 3/15/24 12:01, Peter Xu wrote:
> > > On Fri, Mar 15, 2024 at 11:17:45AM +0100, Cédric Le Goater wrote:
> > > > > migrate_set_state is also unintuitive because it ignores invalid state
> > > > > transitions and we've been using that property to deal with special
> > > > > states such as POSTCOPY_PAUSED and FAILED:
> > > > >
> > > > > - After the migration goes into POSTCOPY_PAUSED, the resumed
> > > > > migration's
> > > > > migrate_init() will try to set the state NONE->SETUP, which is not
> > > > > valid.
> > > > >
> > > > > - After save_setup fails, the migration goes into FAILED, but
> > > > > wait_unplug
> > > > > will try to transition SETUP->ACTIVE, which is also not valid.
> > > > >
> > > >
> > > > I am not sure I understand what the plan is. Both solutions are
> > > > problematic
> > > > regarding the state transitions.
> > > >
> > > > Should we consider that waiting for failover devices to unplug is an
> > > > internal
> > > > step of the SETUP phase not transitioning to ACTIVE ?
> > >
> > > If to unblock this series, IIUC the simplest solution is to do what
> > > Fabiano
> > > suggested, that we move qemu_savevm_wait_unplug() to be before the check
> > > of
> > > setup() ret.
> >
> > The simplest is IMHO moving qemu_savevm_wait_unplug() before
> > qemu_savevm_state_setup() and leave patch 10 is unchanged. See
> > below the extra patch. It looks much cleaner than what we have
> > today.
> >
> > > In that case, the state change in qemu_savevm_wait_unplug()
> > > should be benign and we should see a super small window it became ACTIVE
> > > but then it should be FAILED (and IIUC the patch itself will need to use
> > > ACTIVE as "old_state", not SETUP anymore).
> >
> > OK. I will give it a try to compare.
>
> Here's the alternative solution. SETUP state failures are handled after
> transitioning to ACTIVE state, which is unfortunate but probably harmless.
> I guess it's OK.
This also looks good to me, thanks.
One trivial early comment is in this case we can introduce a helper to
cover both setup() calls and UNPLUG waits and dedup the two paths.
>
> Thanks,
>
> C.
>
>
>
> Signed-off-by: Cédric Le Goater
> ---
> migration/savevm.h| 2 +-
> migration/migration.c | 29 +++--
> migration/savevm.c| 26 +++---
> 3 files changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/migration/savevm.h b/migration/savevm.h
> index
> 74669733dd63a080b765866c703234a5c4939223..9ec96a995c93a42aad621595f0ed58596c532328
> 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -32,7 +32,7 @@
> bool qemu_savevm_state_blocked(Error **errp);
> void qemu_savevm_non_migratable_list(strList **reasons);
> int qemu_savevm_state_prepare(Error **errp);
> -void qemu_savevm_state_setup(QEMUFile *f);
> +int qemu_savevm_state_setup(QEMUFile *f, Error **errp);
> bool qemu_savevm_state_guest_unplug_pending(void);
> int qemu_savevm_state_resume_prepare(MigrationState *s);
> void qemu_savevm_state_header(QEMUFile *f);
> diff --git a/migration/migration.c b/migration/migration.c
> index
> 644e073b7dcc70cb2bdaa9c975ba478952465ff4..0704ad6226df61f2f15bd81a2897f9946d601ca7
> 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3427,6 +3427,8 @@ static void *migration_thread(void *opaque)
> int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> MigThrError thr_error;
> bool urgent = false;
> +Error *local_err = NULL;
> +int ret;
> thread = migration_threads_add("live_migration", qemu_get_thread_id());
> @@ -3470,12 +3472,24 @@ static void *migration_thread(void *opaque)
> }
> bql_lock();
> -qemu_savevm_state_setup(s->to_dst_file);
> +ret = qemu_savevm_state_setup(s->to_dst_file, _err);
> bql_unlock();
> qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> MIGRATION_STATUS_ACTIVE);
> +/*
> + * Handle SETUP failures after waiting for virtio-net-failover
> + * devices to unplug. This to preserve migration state transitions.
> + */
> +if (ret) {
> +migrate_set_error(s, local_err);
> +error_free(local_err);
> +migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
> + MIGRATION_STATUS_FAILED);
> +goto out;
> +}
> +
> s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> trace_migration_thread_setup_complete();
> @@ -3549,6 +3563,8 @@ static void *bg_migration_thread(void *opaque)
> MigThrError thr_error;
> QEMUFile *fb;
> bool early_fail = true;
> +Error *local_err = NULL;
> +int ret;
> rcu_register_thread();
> object_ref(OBJECT(s));
> @@ -3582,12 +3598,20 @@ static void *bg_migration_thread(void *opaque)
> bql_lock();
> qemu_savevm_state_header(s->to_dst_file);
> -
[RFC] Is there a bug in pause_all_vcpus()
During we waited on qemu_pause_cond the bql was unlocked,
the vcpu's state may has been changed by other thread, so
we must request the pause state on all vcpus again.
For example:
Both main loop thread and vCPU thread are allowed to call
pause_all_vcpus(), and in general resume_all_vcpus() is called
after it.
Thus there is possibility that during thread T1 waits on
qemu_pause_cond, other thread has called pause_all_vcpus() and
resume_all_vcpus(), then thread T1 will stuck, because the condition
all_vcpus_paused() is always false.
PS:
I hit this bug when I test the RFC patch of ARM vCPU hotplug feature.
This patch is not verified!!! just for RFC.
Signed-off-by: Keqian Zhu
---
system/cpus.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/system/cpus.c b/system/cpus.c
index 68d161d96b..24ac8085cd 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -571,12 +571,14 @@ static bool all_vcpus_paused(void)
return true;
}
-void pause_all_vcpus(void)
+static void request_pause_all_vcpus(void)
{
CPUState *cpu;
-qemu_clock_enable(QEMU_CLOCK_VIRTUAL, false);
CPU_FOREACH(cpu) {
+if (cpu->stopped) {
+continue;
+}
if (qemu_cpu_is_self(cpu)) {
qemu_cpu_stop(cpu, true);
} else {
@@ -584,6 +586,14 @@ void pause_all_vcpus(void)
qemu_cpu_kick(cpu);
}
}
+}
+
+void pause_all_vcpus(void)
+{
+CPUState *cpu;
+
+qemu_clock_enable(QEMU_CLOCK_VIRTUAL, false);
+request_pause_all_vcpus();
/* We need to drop the replay_lock so any vCPU threads woken up
* can finish their replay tasks
@@ -592,9 +602,11 @@ void pause_all_vcpus(void)
while (!all_vcpus_paused()) {
qemu_cond_wait(_pause_cond, );
-CPU_FOREACH(cpu) {
-qemu_cpu_kick(cpu);
-}
+/* During we waited on qemu_pause_cond the bql was unlocked,
+ * the vcpu's state may has been changed by other thread, so
+ * we must request the pause state on all vcpus again.
+ */
+request_pause_all_vcpus();
}
bql_unlock();
--
2.36.1
Re: [PATCH v4 10/25] migration: Add Error** argument to qemu_savevm_state_setup()
On 3/15/24 14:11, Peter Xu wrote: On Fri, Mar 15, 2024 at 01:20:49PM +0100, Cédric Le Goater wrote: +static void qemu_savevm_wait_unplug(MigrationState *s, int state) One more trivial comment: I'd even consider dropping "state" altogether, as this should be the only state this function should be invoked. So we can perhaps assert it instead of passing it over? Yes. If you prefer this implementation I will change. Thanks, C.
Re: [PATCH v4 10/25] migration: Add Error** argument to qemu_savevm_state_setup()
On 3/15/24 14:09, Peter Xu wrote:
On Fri, Mar 15, 2024 at 01:20:49PM +0100, Cédric Le Goater wrote:
On 3/15/24 12:01, Peter Xu wrote:
On Fri, Mar 15, 2024 at 11:17:45AM +0100, Cédric Le Goater wrote:
migrate_set_state is also unintuitive because it ignores invalid state
transitions and we've been using that property to deal with special
states such as POSTCOPY_PAUSED and FAILED:
- After the migration goes into POSTCOPY_PAUSED, the resumed migration's
migrate_init() will try to set the state NONE->SETUP, which is not
valid.
- After save_setup fails, the migration goes into FAILED, but wait_unplug
will try to transition SETUP->ACTIVE, which is also not valid.
I am not sure I understand what the plan is. Both solutions are problematic
regarding the state transitions.
Should we consider that waiting for failover devices to unplug is an internal
step of the SETUP phase not transitioning to ACTIVE ?
If to unblock this series, IIUC the simplest solution is to do what Fabiano
suggested, that we move qemu_savevm_wait_unplug() to be before the check of
setup() ret.
The simplest is IMHO moving qemu_savevm_wait_unplug() before
qemu_savevm_state_setup() and leave patch 10 is unchanged. See
below the extra patch. It looks much cleaner than what we have
today.
Yes it looks cleaner indeed, it's just that then we'll have one more
possible state conversions like SETUP->UNPLUG->SETUP. I'd say it's fine,
but let's also copy Laruent and Laine if it's going to be posted formally.
OK. I just sent the alternative implementation. The code looks a little
ugly :
bql_lock(); |
qemu_savevm_state_header(s->to_dst_file); |
ret = qemu_savevm_state_setup(s->to_dst_file, _err);| in
SETUP state
bql_unlock(); |
|
qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,| SETUP
-> ACTIVE transition
MIGRATION_STATUS_ACTIVE); |
|
/*|
* Handle SETUP failures after waiting for virtio-net-failover|
* devices to unplug. This to preserve migration state transitions. |
*/ |
if (ret) {|
migrate_set_error(s, local_err); |
handling SETUP errors in ACTIVE
error_free(local_err);|
migrate_set_state(>state, MIGRATION_STATUS_ACTIVE, |
MIGRATION_STATUS_FAILED); |
goto fail_setup; |
} |
|
s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; |
SETUP duration
|
trace_migration_thread_setup_complete(); |
SETUP trace event
Thanks,
C.
Re: [PATCH v3 1/7] qga: guest-get-fsinfo: add optional 'total-bytes-root' field
On 3/15/24 15:44, Markus Armbruster wrote:
> [?? ??? ? ?? ?? arm...@redhat.com. ???, ?? ??? ?,
> ?? ?? https://aka.ms/LearnAboutSenderIdentification ]
>
> Andrey Drobyshev writes:
>
>> Since the commit 25b5ff1a86 ("qga: add mountpoint usage info to
>> GuestFilesystemInfo") we have 2 values reported in guest-get-fsinfo:
>> used = (f_blocks - f_bfree), total = (f_blocks - f_bfree + f_bavail) as
>> returned by statvfs(3). While on Windows guests that's all we can get
>> with GetDiskFreeSpaceExA(), on POSIX guests we might also be interested in
>> total file system size, as it's visible for root user. Let's add an
>> optional field 'total-bytes-root' to GuestFilesystemInfo struct, which'd
>> only be reported on POSIX and represent f_blocks value as returned by
>> statvfs(3).
>>
>> While here, let's document better where those values come from in both
>> POSIX and Windows.
>>
>> Signed-off-by: Andrey Drobyshev
>
> [...]
>
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index b8efe31897..093a5ab602 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -1031,8 +1031,18 @@
>> # @type: file system type string
>> #
>> # @used-bytes: file system used bytes (since 3.0)
>> +# * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by statvfs(3)
>> +# * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as returned
>> +# by GetDiskFreeSpaceEx()
>> #
>> # @total-bytes: non-root file system total bytes (since 3.0)
>> +# * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
>> +# statvfs(3)
>> +# * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
>> +#
>> +# @total-bytes-root: total file system size in bytes (as visible for a
>> +# priviledged user) (since 8.3)
>
> privileged
>
>> +# * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
>> #
>> # @disk: an array of disk hardware information that the volume lies
>> # on, which may be empty if the disk type is not supported
>> @@ -1042,7 +1052,7 @@
>> { 'struct': 'GuestFilesystemInfo',
>>'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
>> '*used-bytes': 'uint64', '*total-bytes': 'uint64',
>> - 'disk': ['GuestDiskAddress']} }
>> + '*total-bytes-root': 'uint64', 'disk': ['GuestDiskAddress']} }
>>
>> ##
>> # @guest-get-fsinfo:
>
> Fails to build the manual:
>
> qga/qapi-schema.json:1019:Unexpected indentation.
>
> To fix, add blank lines before the lists, like this:
>
># @used-bytes: file system used bytes (since 3.0)
>#
># * POSIX: (f_blocks - f_bfree) * f_frsize, as returned by
># statvfs(3)
># * Windows: (TotalNumberOfBytes - TotalNumberOfFreeBytes), as
># returned by GetDiskFreeSpaceEx()
>#
># @total-bytes: non-root file system total bytes (since 3.0)
>#
># * POSIX: (f_blocks - f_bfree + f_bavail) * f_frsize, as returned by
># statvfs(3)
># * Windows: TotalNumberOfBytes, as returned by GetDiskFreeSpaceEx()
>#
># @total-bytes-root: total file system size in bytes (as visible for a
># privileged user) (since 8.3)
>#
># * POSIX only: (f_blocks * f_frsize), returned by statvfs(3)
>#
>
> Yes, reST can be quite annoying.
>
For some reason in my environment build doesn't fail and file
build/docs/qemu-ga-ref.7 is produced. However lists aren't indeed
formatted properly. I'll wait for other patches to be reviewed and fix
that in v4. Thank you for noticing.
Re: [PATCH v4 10/25] migration: Add Error** argument to qemu_savevm_state_setup()
On 3/15/24 13:20, Cédric Le Goater wrote:
On 3/15/24 12:01, Peter Xu wrote:
On Fri, Mar 15, 2024 at 11:17:45AM +0100, Cédric Le Goater wrote:
migrate_set_state is also unintuitive because it ignores invalid state
transitions and we've been using that property to deal with special
states such as POSTCOPY_PAUSED and FAILED:
- After the migration goes into POSTCOPY_PAUSED, the resumed migration's
migrate_init() will try to set the state NONE->SETUP, which is not
valid.
- After save_setup fails, the migration goes into FAILED, but wait_unplug
will try to transition SETUP->ACTIVE, which is also not valid.
I am not sure I understand what the plan is. Both solutions are problematic
regarding the state transitions.
Should we consider that waiting for failover devices to unplug is an internal
step of the SETUP phase not transitioning to ACTIVE ?
If to unblock this series, IIUC the simplest solution is to do what Fabiano
suggested, that we move qemu_savevm_wait_unplug() to be before the check of
setup() ret.
The simplest is IMHO moving qemu_savevm_wait_unplug() before
qemu_savevm_state_setup() and leave patch 10 is unchanged. See
below the extra patch. It looks much cleaner than what we have
today.
In that case, the state change in qemu_savevm_wait_unplug()
should be benign and we should see a super small window it became ACTIVE
but then it should be FAILED (and IIUC the patch itself will need to use
ACTIVE as "old_state", not SETUP anymore).
OK. I will give it a try to compare.
Here's the alternative solution. SETUP state failures are handled after
transitioning to ACTIVE state, which is unfortunate but probably harmless.
I guess it's OK.
Thanks,
C.
Signed-off-by: Cédric Le Goater
---
migration/savevm.h| 2 +-
migration/migration.c | 29 +++--
migration/savevm.c| 26 +++---
3 files changed, 43 insertions(+), 14 deletions(-)
diff --git a/migration/savevm.h b/migration/savevm.h
index
74669733dd63a080b765866c703234a5c4939223..9ec96a995c93a42aad621595f0ed58596c532328
100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -32,7 +32,7 @@
bool qemu_savevm_state_blocked(Error **errp);
void qemu_savevm_non_migratable_list(strList **reasons);
int qemu_savevm_state_prepare(Error **errp);
-void qemu_savevm_state_setup(QEMUFile *f);
+int qemu_savevm_state_setup(QEMUFile *f, Error **errp);
bool qemu_savevm_state_guest_unplug_pending(void);
int qemu_savevm_state_resume_prepare(MigrationState *s);
void qemu_savevm_state_header(QEMUFile *f);
diff --git a/migration/migration.c b/migration/migration.c
index
644e073b7dcc70cb2bdaa9c975ba478952465ff4..0704ad6226df61f2f15bd81a2897f9946d601ca7
100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3427,6 +3427,8 @@ static void *migration_thread(void *opaque)
int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
MigThrError thr_error;
bool urgent = false;
+Error *local_err = NULL;
+int ret;
thread = migration_threads_add("live_migration", qemu_get_thread_id());
@@ -3470,12 +3472,24 @@ static void *migration_thread(void *opaque)
}
bql_lock();
-qemu_savevm_state_setup(s->to_dst_file);
+ret = qemu_savevm_state_setup(s->to_dst_file, _err);
bql_unlock();
qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_ACTIVE);
+/*
+ * Handle SETUP failures after waiting for virtio-net-failover
+ * devices to unplug. This to preserve migration state transitions.
+ */
+if (ret) {
+migrate_set_error(s, local_err);
+error_free(local_err);
+migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+goto out;
+}
+
s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
trace_migration_thread_setup_complete();
@@ -3549,6 +3563,8 @@ static void *bg_migration_thread(void *opaque)
MigThrError thr_error;
QEMUFile *fb;
bool early_fail = true;
+Error *local_err = NULL;
+int ret;
rcu_register_thread();
object_ref(OBJECT(s));
@@ -3582,12 +3598,20 @@ static void *bg_migration_thread(void *opaque)
bql_lock();
qemu_savevm_state_header(s->to_dst_file);
-qemu_savevm_state_setup(s->to_dst_file);
+ret = qemu_savevm_state_setup(s->to_dst_file, _err);
bql_unlock();
qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_ACTIVE);
+if (ret) {
+migrate_set_error(s, local_err);
+error_free(local_err);
+migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+goto fail_setup;
+}
+
s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
trace_migration_thread_setup_complete();
@@ -3656,6 +3680,7 @@ fail:
bql_unlock();
}
+fail_setup:
Re: [PATCH 03/12] uefi-test-tools: Add support for python based build script
> +Build/bios-tables-test.%.efi: > + $(PYTHON) ../../roms/edk2-build.py --config uefi-test-build.config Adding '--match $*' will build one arch instead of all.
Re: [PATCH 02/12] uefi-test-tools/UefiTestToolsPkg: Add RISC-V support
On Fri, Mar 15, 2024 at 06:35:09PM +0530, Sunil V L wrote: > Enable building the test application for RISC-V with appropriate > dependencies updated. > > Signed-off-by: Sunil V L > --- > tests/uefi-test-tools/UefiTestToolsPkg/UefiTestToolsPkg.dsc | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) Acked-by: Gerd Hoffmann
Re: [PATCH 01/12] roms/edk2-build.py: Add --module support
On Fri, Mar 15, 2024 at 06:35:08PM +0530, Sunil V L wrote: > UefiTestToolsPkg which should use edk2-build.py needs --module parameter > support. Add this optional parameter handling. I don't think this is needed. By default everything listed in [Components] should be built, which is just that one module we have ;) take care, Gerd
[PATCH for-9.0 v2] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
VDUSE requires that virtqueues are first enabled before the DRIVER_OK
status flag is set; with the current API of the kernel module, it is
impossible to enable the opposite order in our block export code because
userspace is not notified when a virtqueue is enabled.
This requirement also mathces the normal initialisation order as done by
the generic vhost code in QEMU. However, commit 6c482547 accidentally
changed the order for vdpa-dev and broke access to VDUSE devices with
this.
This changes vdpa-dev to use the normal order again and use the standard
vhost callback .vhost_set_vring_enable for this. VDUSE devices can be
used with vdpa-dev again after this fix.
vhost_net intentionally avoided enabling the vrings for vdpa and does
this manually later while it does enable them for other vhost backends.
Reflect this in the vhost_net code and return early for vdpa, so that
the behaviour doesn't change for this device.
Cc: qemu-sta...@nongnu.org
Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
Signed-off-by: Kevin Wolf
---
v2:
- Actually make use of the @enable parameter
- Change vhost_net to preserve the current behaviour
hw/net/vhost_net.c | 10 ++
hw/virtio/vdpa-dev.c | 5 +
hw/virtio/vhost-vdpa.c | 27 +--
hw/virtio/vhost.c | 8 +++-
4 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e8e1661646..fd1a93701a 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable)
VHostNetState *net = get_vhost_net(nc);
const VhostOps *vhost_ops = net->dev.vhost_ops;
+/*
+ * vhost-vdpa network devices need to enable dataplane virtqueues after
+ * DRIVER_OK, so they can recover device state before starting dataplane.
+ * Because of that, we don't enable virtqueues here and leave it to
+ * net/vhost-vdpa.c.
+ */
+if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+return 0;
+}
+
nc->vring_enable = enable;
if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index eb9ecea83b..13e87f06f6 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev,
Error **errp)
s->dev.acked_features = vdev->guest_features;
-ret = vhost_dev_start(>dev, vdev, false);
+ret = vhost_dev_start(>dev, vdev, true);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error starting vhost");
goto err_guest_notifiers;
}
-for (i = 0; i < s->dev.nvqs; ++i) {
-vhost_vdpa_set_vring_ready(>vdpa, i);
-}
s->started = true;
/*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ddae494ca8..401afac2f5 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -886,12 +886,13 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev,
int idx)
return idx;
}
-int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned idx,
+ int enable)
{
struct vhost_dev *dev = v->dev;
struct vhost_vring_state state = {
.index = idx,
-.num = 1,
+.num = enable,
};
int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, );
@@ -899,6 +900,27 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
unsigned idx)
return r;
}
+static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable)
+{
+struct vhost_vdpa *v = dev->opaque;
+unsigned int i;
+int ret;
+
+for (i = 0; i < dev->nvqs; ++i) {
+ret = vhost_vdpa_set_vring_enable_one(v, i, enable);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
+
+int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+{
+return vhost_vdpa_set_vring_enable_one(v, idx, 1);
+}
+
static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
int fd)
{
@@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = {
.vhost_set_features = vhost_vdpa_set_features,
.vhost_reset_device = vhost_vdpa_reset_device,
.vhost_get_vq_index = vhost_vdpa_get_vq_index,
+.vhost_set_vring_enable = vhost_vdpa_set_vring_enable,
.vhost_get_config = vhost_vdpa_get_config,
.vhost_set_config = vhost_vdpa_set_config,
.vhost_requires_shm_log = NULL,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..decfb85184 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1984,7 +1984,13 @@ static int vhost_dev_set_vring_enable(struct vhost_dev
*hdev, int enable)
return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable);
}
-/* Host notifiers must be enabled at this point. */
+/*
+ * Host notifiers must be enabled at
