From 917ddc27d86ae427d2aa7ff2d19eb7fdb642b68e Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Fri, 23 Jul 2021 14:01:56 +0200 Subject: [PATCH 1/9] meson: fix dependencies for modinfo #2 modinfo runs the preprocessor and therefore needs all generated input files to be there. The "depends" clause does not work in Meson 0.55.3, so for now use "input". Part #2: Update the rule for target-specific modules too. Signed-off-by: Gerd Hoffmann Message-Id: <20210723120156.1183920-1-kraxel@redhat.com> Signed-off-by: Paolo Bonzini --- meson.build | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index df5094e563..f2e148eaf9 100644 --- a/meson.build +++ b/meson.build @@ -2373,9 +2373,9 @@ foreach d, list : target_modules # FIXME: Should use sl.extract_all_objects(recursive: true) too. modinfo_files += custom_target(module_name + '.modinfo', output: module_name + '.modinfo', - input: target_module_ss.sources(), + input: target_module_ss.sources() + genh, capture: true, - command: [modinfo_collect, '--target', target, '@INPUT@']) + command: [modinfo_collect, '--target', target, target_module_ss.sources()]) endif endif endforeach From 3407259b20ccb5f53183bc50605da6f229dc2de2 Mon Sep 17 00:00:00 2001 From: Lara Lazier Date: Fri, 23 Jul 2021 13:27:40 +0200 Subject: [PATCH 2/9] target/i386: Added consistency checks for CR3 All MBZ in CR3 must be zero (APM2 15.5) Added checks in both helper_vmrun and helper_write_crN. When EFER.LMA is zero the upper 32 bits needs to be zeroed. Signed-off-by: Lara Lazier Message-Id: <20210723112740.45962-1-laramglazier@gmail.com> Signed-off-by: Paolo Bonzini --- target/i386/tcg/sysemu/misc_helper.c | 7 +++++++ target/i386/tcg/sysemu/svm_helper.c | 10 +++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/target/i386/tcg/sysemu/misc_helper.c b/target/i386/tcg/sysemu/misc_helper.c index a2af2c9bba..d347af2a99 100644 --- a/target/i386/tcg/sysemu/misc_helper.c +++ b/target/i386/tcg/sysemu/misc_helper.c @@ -96,6 +96,13 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0) cpu_x86_update_cr0(env, t0); break; case 3: + if ((env->efer & MSR_EFER_LMA) && + (t0 & ((~0UL) << env_archcpu(env)->phys_bits))) { + cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); + } + if (!(env->efer & MSR_EFER_LMA)) { + t0 &= 0xffffffffUL; + } cpu_x86_update_cr3(env, t0); break; case 4: diff --git a/target/i386/tcg/sysemu/svm_helper.c b/target/i386/tcg/sysemu/svm_helper.c index 4d64ec378e..145511d635 100644 --- a/target/i386/tcg/sysemu/svm_helper.c +++ b/target/i386/tcg/sysemu/svm_helper.c @@ -120,6 +120,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) uint32_t int_ctl; uint32_t asid; uint64_t new_cr0; + uint64_t new_cr3; uint64_t new_cr4; cpu_svm_check_intercept_param(env, SVM_EXIT_VMRUN, 0, GETPC()); @@ -261,6 +262,11 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) if ((new_cr0 & CR0_NW_MASK) && !(new_cr0 & CR0_CD_MASK)) { cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); } + new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr3)); + if ((env->efer & MSR_EFER_LMA) && + (new_cr3 & ((~0UL) << cpu->phys_bits))) { + cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); + } new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr4)); if (new_cr4 & cr4_reserved_bits(env)) { cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); @@ -271,9 +277,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) cpu_x86_update_cr0(env, new_cr0); cpu_x86_update_cr4(env, new_cr4); - cpu_x86_update_cr3(env, x86_ldq_phys(cs, - env->vm_vmcb + offsetof(struct vmcb, - save.cr3))); + cpu_x86_update_cr3(env, new_cr3); env->cr[2] = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, save.cr2)); int_ctl = x86_ldl_phys(cs, From 5b8978d8042660de35b2c67c62ffeb6b42ff441e Mon Sep 17 00:00:00 2001 From: Claudio Fontana Date: Fri, 23 Jul 2021 13:29:21 +0200 Subject: [PATCH 3/9] i386: do not call cpudef-only models functions for max, host, base Some cpu properties have to be set only for cpu models in builtin_x86_defs, registered with x86_register_cpu_model_type, and not for cpu models "base", "max", and the subclass "host". These properties are the ones set by function x86_cpu_apply_props, (also including kvm_default_props, tcg_default_props), and the "vendor" property for the KVM and HVF accelerators. After recent refactoring of cpu, which also affected these properties, they were instead set unconditionally for all x86 cpus. This has been detected as a bug with Nested on AMD with cpu "host", as svm was not turned on by default, due to the wrongful setting of kvm_default_props via x86_cpu_apply_props, which set svm to "off". Rectify the bug introduced in commit "i386: split cpu accelerators" and document the functions that are builtin_x86_defs-only. Signed-off-by: Claudio Fontana Tested-by: Alexander Bulekov Fixes: f5cc5a5c ("i386: split cpu accelerators from cpu.c,"...) Resolves: https://gitlab.com/qemu-project/qemu/-/issues/477 Message-Id: <20210723112921.12637-1-cfontana@suse.de> Signed-off-by: Paolo Bonzini --- target/i386/cpu.c | 19 ++++++- target/i386/host-cpu.c | 13 +++-- target/i386/kvm/kvm-cpu.c | 105 ++++++++++++++++++++------------------ target/i386/tcg/tcg-cpu.c | 11 ++-- 4 files changed, 89 insertions(+), 59 deletions(-) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 48b55ebd0a..edb97ebbbe 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -4919,6 +4919,9 @@ static uint64_t x86_cpu_get_supported_feature_word(FeatureWord w, return r; } +/* + * Only for builtin_x86_defs models initialized with x86_register_cpudef_types. + */ void x86_cpu_apply_props(X86CPU *cpu, PropValue *props) { PropValue *pv; @@ -4931,7 +4934,11 @@ void x86_cpu_apply_props(X86CPU *cpu, PropValue *props) } } -/* Apply properties for the CPU model version specified in model */ +/* + * Apply properties for the CPU model version specified in model. + * Only for builtin_x86_defs models initialized with x86_register_cpudef_types. + */ + static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUModel *model) { const X86CPUVersionDefinition *vdef; @@ -4960,7 +4967,9 @@ static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUModel *model) assert(vdef->version == version); } -/* Load data from X86CPUDefinition into a X86CPU object +/* + * Load data from X86CPUDefinition into a X86CPU object. + * Only for builtin_x86_defs models initialized with x86_register_cpudef_types. */ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model) { @@ -5051,6 +5060,12 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model) type_register(&ti); } + +/* + * register builtin_x86_defs; + * "max", "base" and subclasses ("host") are not registered here. + * See x86_cpu_register_types for all model registrations. + */ static void x86_register_cpudef_types(const X86CPUDefinition *def) { X86CPUModel *m; diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c index 4ea9e354ea..10f8aba86e 100644 --- a/target/i386/host-cpu.c +++ b/target/i386/host-cpu.c @@ -150,13 +150,16 @@ void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping) void host_cpu_instance_init(X86CPU *cpu) { - uint32_t ebx = 0, ecx = 0, edx = 0; - char vendor[CPUID_VENDOR_SZ + 1]; + X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu); - host_cpuid(0, 0, NULL, &ebx, &ecx, &edx); - x86_cpu_vendor_words2str(vendor, ebx, edx, ecx); + if (xcc->model) { + uint32_t ebx = 0, ecx = 0, edx = 0; + char vendor[CPUID_VENDOR_SZ + 1]; - object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort); + host_cpuid(0, 0, NULL, &ebx, &ecx, &edx); + x86_cpu_vendor_words2str(vendor, ebx, edx, ecx); + object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort); + } } void host_cpu_max_instance_init(X86CPU *cpu) diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c index bbe817764d..d95028018e 100644 --- a/target/i386/kvm/kvm-cpu.c +++ b/target/i386/kvm/kvm-cpu.c @@ -52,47 +52,6 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp) return host_cpu_realizefn(cs, errp); } -/* - * KVM-specific features that are automatically added/removed - * from all CPU models when KVM is enabled. - * - * NOTE: features can be enabled by default only if they were - * already available in the oldest kernel version supported - * by the KVM accelerator (see "OS requirements" section at - * docs/system/target-i386.rst) - */ -static PropValue kvm_default_props[] = { - { "kvmclock", "on" }, - { "kvm-nopiodelay", "on" }, - { "kvm-asyncpf", "on" }, - { "kvm-steal-time", "on" }, - { "kvm-pv-eoi", "on" }, - { "kvmclock-stable-bit", "on" }, - { "x2apic", "on" }, - { "kvm-msi-ext-dest-id", "off" }, - { "acpi", "off" }, - { "monitor", "off" }, - { "svm", "off" }, - { NULL, NULL }, -}; - -void x86_cpu_change_kvm_default(const char *prop, const char *value) -{ - PropValue *pv; - for (pv = kvm_default_props; pv->prop; pv++) { - if (!strcmp(pv->prop, prop)) { - pv->value = value; - break; - } - } - - /* - * It is valid to call this function only for properties that - * are already present in the kvm_default_props table. - */ - assert(pv->prop); -} - static bool lmce_supported(void) { uint64_t mce_cap = 0; @@ -150,22 +109,70 @@ static void kvm_cpu_xsave_init(void) } } +/* + * KVM-specific features that are automatically added/removed + * from cpudef models when KVM is enabled. + * Only for builtin_x86_defs models initialized with x86_register_cpudef_types. + * + * NOTE: features can be enabled by default only if they were + * already available in the oldest kernel version supported + * by the KVM accelerator (see "OS requirements" section at + * docs/system/target-i386.rst) + */ +static PropValue kvm_default_props[] = { + { "kvmclock", "on" }, + { "kvm-nopiodelay", "on" }, + { "kvm-asyncpf", "on" }, + { "kvm-steal-time", "on" }, + { "kvm-pv-eoi", "on" }, + { "kvmclock-stable-bit", "on" }, + { "x2apic", "on" }, + { "kvm-msi-ext-dest-id", "off" }, + { "acpi", "off" }, + { "monitor", "off" }, + { "svm", "off" }, + { NULL, NULL }, +}; + +/* + * Only for builtin_x86_defs models initialized with x86_register_cpudef_types. + */ +void x86_cpu_change_kvm_default(const char *prop, const char *value) +{ + PropValue *pv; + for (pv = kvm_default_props; pv->prop; pv++) { + if (!strcmp(pv->prop, prop)) { + pv->value = value; + break; + } + } + + /* + * It is valid to call this function only for properties that + * are already present in the kvm_default_props table. + */ + assert(pv->prop); +} + static void kvm_cpu_instance_init(CPUState *cs) { X86CPU *cpu = X86_CPU(cs); + X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu); host_cpu_instance_init(cpu); - if (!kvm_irqchip_in_kernel()) { - x86_cpu_change_kvm_default("x2apic", "off"); - } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) { - x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on"); + if (xcc->model) { + /* only applies to builtin_x86_defs cpus */ + if (!kvm_irqchip_in_kernel()) { + x86_cpu_change_kvm_default("x2apic", "off"); + } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) { + x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on"); + } + + /* Special cases not set in the X86CPUDefinition structs: */ + x86_cpu_apply_props(cpu, kvm_default_props); } - /* Special cases not set in the X86CPUDefinition structs: */ - - x86_cpu_apply_props(cpu, kvm_default_props); - if (cpu->max_features) { kvm_cpu_max_instance_init(cpu); } diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c index 238e3a9395..93a79a5741 100644 --- a/target/i386/tcg/tcg-cpu.c +++ b/target/i386/tcg/tcg-cpu.c @@ -111,7 +111,8 @@ static void tcg_cpu_xsave_init(void) } /* - * TCG-specific defaults that override all CPU models when using TCG + * TCG-specific defaults that override cpudef models when using TCG. + * Only for builtin_x86_defs models initialized with x86_register_cpudef_types. */ static PropValue tcg_default_props[] = { { "vme", "off" }, @@ -121,8 +122,12 @@ static PropValue tcg_default_props[] = { static void tcg_cpu_instance_init(CPUState *cs) { X86CPU *cpu = X86_CPU(cs); - /* Special cases not set in the X86CPUDefinition structs: */ - x86_cpu_apply_props(cpu, tcg_default_props); + X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu); + + if (xcc->model) { + /* Special cases not set in the X86CPUDefinition structs: */ + x86_cpu_apply_props(cpu, tcg_default_props); + } tcg_cpu_xsave_init(); } From 4ade3ea145261fb6ae6c80b284f6912f3f7b87be Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 23 Jul 2021 12:05:30 +0200 Subject: [PATCH 4/9] MAINTAINERS: Replace Eduardo as "Host Memory Backends" maintainer Edurdo asked me to take over co-maintaining "Host Memory Backends" with Igor, as Eduardo has plenty of other things to look after. Thanks a lot Eduardo for your excellent work in the past! Cc: Peter Maydell Cc: Eduardo Habkost Cc: Igor Mammedov Cc: Paolo Bonzini Signed-off-by: David Hildenbrand Acked-by: Igor Mammedov Message-Id: <20210723100532.27353-2-david@redhat.com> Signed-off-by: Paolo Bonzini --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 4256ad1adb..420c8a48a1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2547,7 +2547,7 @@ S: Maintained F: net/netmap.c Host Memory Backends -M: Eduardo Habkost +M: David Hildenbrand M: Igor Mammedov S: Maintained F: backends/hostmem*.c From 07b315ba92f508cd63ca0adeab58fbb13d0b5585 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 23 Jul 2021 12:05:31 +0200 Subject: [PATCH 5/9] MAINTAINERS: Add Peter Xu and myself as co-maintainer of "Memory API" Peter and myself volunteered to help out co-maintaining "Memory API" with Paolo, so let's update the MAINTAINERS file. Cc: Peter Maydell Cc: Paolo Bonzini Cc: Peter Xu Signed-off-by: David Hildenbrand Message-Id: <20210723100532.27353-3-david@redhat.com> Signed-off-by: Paolo Bonzini --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 420c8a48a1..190a90b541 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2452,6 +2452,8 @@ F: tests/tcg/multiarch/gdbstub/ Memory API M: Paolo Bonzini +M: Peter Xu +M: David Hildenbrand S: Supported F: include/exec/ioport.h F: include/exec/memop.h From 9f04dd7f5a3dd6d9a4c34f4882dd1ef0b7936a2f Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Fri, 23 Jul 2021 12:05:32 +0200 Subject: [PATCH 6/9] MAINTAINERS: Add memory_mapping.h and memory_mapping.c to "Memory API" Both files logically belong to "Memory API" and are not yet listed anywhere else explicitly. Let's add them to "Memory API". Cc: Peter Maydell Cc: Paolo Bonzini Cc: Peter Xu Signed-off-by: David Hildenbrand Acked-by: Peter Xu Message-Id: <20210723100532.27353-4-david@redhat.com> Signed-off-by: Paolo Bonzini --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 190a90b541..445f7fe2d1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2460,9 +2460,11 @@ F: include/exec/memop.h F: include/exec/memory.h F: include/exec/ram_addr.h F: include/exec/ramblock.h +F: include/sysemu/memory_mapping.h F: softmmu/dma-helpers.c F: softmmu/ioport.c F: softmmu/memory.c +F: softmmu/memory_mapping.c F: softmmu/physmem.c F: include/exec/memory-internal.h F: scripts/coccinelle/memory-region-housekeeping.cocci From eafadbbbac06a8d72baa976f4d3c42b0e5f8cfc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 23 Jul 2021 12:30:51 +0100 Subject: [PATCH 7/9] gitlab: only let pages be published from default branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitLab will happily publish pages generated by the latest CI pipeline from any branch: https://docs.gitlab.com/ee/user/project/pages/introduction.html "Remember that GitLab Pages are by default branch/tag agnostic and their deployment relies solely on what you specify in .gitlab-ci.yml. You can limit the pages job with the only parameter, whenever a new commit is pushed to a branch used specifically for your pages." The current "pages" job is not limited, so it is happily publishing docs content from any branch/tag in qemu.git that gets pushed to. This means we're potentially publishing from the "staging" branch or worse from outdated "stable-NNN" branches This change restricts it to only publish from the default branch in the main repository. For contributor forks, however, we allow it to publish from any branch, since users will have arbitrarily named topic branches in flight at any time. Signed-off-by: Daniel P. Berrangé Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210723113051.2792799-1-berrange@redhat.com> Signed-off-by: Paolo Bonzini --- .gitlab-ci.d/buildtest.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml index 89df51517c..80b57b7082 100644 --- a/.gitlab-ci.d/buildtest.yml +++ b/.gitlab-ci.d/buildtest.yml @@ -663,6 +663,17 @@ build-tools-and-docs-debian: # Prepare for GitLab pages deployment. Anything copied into the # "public" directory will be deployed to $USER.gitlab.io/$PROJECT +# +# GitLab publishes from any branch that triggers a CI pipeline +# +# For the main repo we don't want to publish from 'staging' +# since that content may not be pushed, nor do we wish to +# publish from 'stable-NNN' branches as that content is outdated. +# Thus we restrict to just the default branch +# +# For contributor forks we want to publish from any repo so +# that users can see the results of their commits, regardless +# of what topic branch they're currently using pages: image: $CI_REGISTRY_IMAGE/qemu/debian-amd64:latest stage: test @@ -681,3 +692,10 @@ pages: artifacts: paths: - public + rules: + - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH' + when: on_success + - if: '$CI_PROJECT_NAMESPACE == "qemu-project"' + when: never + - if: '$CI_PROJECT_NAMESPACE != "qemu-project"' + when: on_success From 18fa3ebc45262cebc7c9f0ba6f717452bdc51db7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sun, 18 Jul 2021 08:49:22 +0200 Subject: [PATCH 8/9] qapi: introduce forwarding visitor This new adaptor visitor takes a single field of the adaptee, and exposes it with a different name. This will be used for QOM alias properties. Alias targets can of course have a different name than the alias property itself (e.g. a machine's pflash0 might be an alias of a property named 'drive'). When the target's getter or setter invokes the visitor, it will use a different name than what the caller expects, and the visitor will not be able to find it (or will consume erroneously). The solution is for alias getters and setters to wrap the incoming visitor, and forward the sole field that the target is expecting while renaming it appropriately. Reviewed-by: Eric Blake Signed-off-by: Paolo Bonzini --- include/qapi/forward-visitor.h | 27 +++ qapi/meson.build | 1 + qapi/qapi-forward-visitor.c | 326 ++++++++++++++++++++++++++++++ tests/unit/meson.build | 1 + tests/unit/test-forward-visitor.c | 197 ++++++++++++++++++ 5 files changed, 552 insertions(+) create mode 100644 include/qapi/forward-visitor.h create mode 100644 qapi/qapi-forward-visitor.c create mode 100644 tests/unit/test-forward-visitor.c diff --git a/include/qapi/forward-visitor.h b/include/qapi/forward-visitor.h new file mode 100644 index 0000000000..50fb3e9d50 --- /dev/null +++ b/include/qapi/forward-visitor.h @@ -0,0 +1,27 @@ +/* + * Forwarding visitor + * + * Copyright Red Hat, Inc. 2021 + * + * Author: Paolo Bonzini + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#ifndef FORWARD_VISITOR_H +#define FORWARD_VISITOR_H + +#include "qapi/visitor.h" + +typedef struct ForwardFieldVisitor ForwardFieldVisitor; + +/* + * The forwarding visitor only expects a single name, @from, to be passed for + * toplevel fields. It is converted to @to and forwarded to the @target visitor. + * Calls within a struct are forwarded without changing the name. + */ +Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to); + +#endif diff --git a/qapi/meson.build b/qapi/meson.build index 376f4ceafe..c356a385e3 100644 --- a/qapi/meson.build +++ b/qapi/meson.build @@ -2,6 +2,7 @@ util_ss.add(files( 'opts-visitor.c', 'qapi-clone-visitor.c', 'qapi-dealloc-visitor.c', + 'qapi-forward-visitor.c', 'qapi-util.c', 'qapi-visit-core.c', 'qobject-input-visitor.c', diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c new file mode 100644 index 0000000000..a4b111e22a --- /dev/null +++ b/qapi/qapi-forward-visitor.c @@ -0,0 +1,326 @@ +/* + * Forward Visitor + * + * Copyright (C) 2021 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "qapi/compat-policy.h" +#include "qapi/error.h" +#include "qapi/forward-visitor.h" +#include "qapi/visitor-impl.h" +#include "qemu/queue.h" +#include "qapi/qmp/qjson.h" +#include "qapi/qmp/qbool.h" +#include "qapi/qmp/qdict.h" +#include "qapi/qmp/qerror.h" +#include "qapi/qmp/qlist.h" +#include "qapi/qmp/qnull.h" +#include "qapi/qmp/qnum.h" +#include "qapi/qmp/qstring.h" +#include "qemu/cutils.h" +#include "qemu/option.h" + +struct ForwardFieldVisitor { + Visitor visitor; + + Visitor *target; + char *from; + char *to; + + int depth; +}; + +static ForwardFieldVisitor *to_ffv(Visitor *v) +{ + return container_of(v, ForwardFieldVisitor, visitor); +} + +static bool forward_field_translate_name(ForwardFieldVisitor *v, const char **name, + Error **errp) +{ + if (v->depth) { + return true; + } + if (g_str_equal(*name, v->from)) { + *name = v->to; + return true; + } + error_setg(errp, QERR_MISSING_PARAMETER, *name); + return false; +} + +static bool forward_field_check_struct(Visitor *v, Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + return visit_check_struct(ffv->target, errp); +} + +static bool forward_field_start_struct(Visitor *v, const char *name, void **obj, + size_t size, Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + if (!visit_start_struct(ffv->target, name, obj, size, errp)) { + return false; + } + ffv->depth++; + return true; +} + +static void forward_field_end_struct(Visitor *v, void **obj) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + assert(ffv->depth); + ffv->depth--; + visit_end_struct(ffv->target, obj); +} + +static bool forward_field_start_list(Visitor *v, const char *name, + GenericList **list, size_t size, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + ffv->depth++; + return visit_start_list(ffv->target, name, list, size, errp); +} + +static GenericList *forward_field_next_list(Visitor *v, GenericList *tail, + size_t size) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + assert(ffv->depth); + return visit_next_list(ffv->target, tail, size); +} + +static bool forward_field_check_list(Visitor *v, Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + assert(ffv->depth); + return visit_check_list(ffv->target, errp); +} + +static void forward_field_end_list(Visitor *v, void **obj) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + assert(ffv->depth); + ffv->depth--; + visit_end_list(ffv->target, obj); +} + +static bool forward_field_start_alternate(Visitor *v, const char *name, + GenericAlternate **obj, size_t size, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + /* + * The name passed to start_alternate is used also in the visit_type_* calls + * that retrieve the alternate's content; so, do not increase depth here. + */ + return visit_start_alternate(ffv->target, name, obj, size, errp); +} + +static void forward_field_end_alternate(Visitor *v, void **obj) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + visit_end_alternate(ffv->target, obj); +} + +static bool forward_field_type_int64(Visitor *v, const char *name, int64_t *obj, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_int64(ffv->target, name, obj, errp); +} + +static bool forward_field_type_uint64(Visitor *v, const char *name, + uint64_t *obj, Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_uint64(ffv->target, name, obj, errp); +} + +static bool forward_field_type_bool(Visitor *v, const char *name, bool *obj, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_bool(ffv->target, name, obj, errp); +} + +static bool forward_field_type_str(Visitor *v, const char *name, char **obj, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_str(ffv->target, name, obj, errp); +} + +static bool forward_field_type_size(Visitor *v, const char *name, uint64_t *obj, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_size(ffv->target, name, obj, errp); +} + +static bool forward_field_type_number(Visitor *v, const char *name, double *obj, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_number(ffv->target, name, obj, errp); +} + +static bool forward_field_type_any(Visitor *v, const char *name, QObject **obj, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_any(ffv->target, name, obj, errp); +} + +static bool forward_field_type_null(Visitor *v, const char *name, + QNull **obj, Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_type_null(ffv->target, name, obj, errp); +} + +static void forward_field_optional(Visitor *v, const char *name, bool *present) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, NULL)) { + *present = false; + return; + } + visit_optional(ffv->target, name, present); +} + +static bool forward_field_deprecated_accept(Visitor *v, const char *name, + Error **errp) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, errp)) { + return false; + } + return visit_deprecated_accept(ffv->target, name, errp); +} + +static bool forward_field_deprecated(Visitor *v, const char *name) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + if (!forward_field_translate_name(ffv, &name, NULL)) { + return false; + } + return visit_deprecated(ffv->target, name); +} + +static void forward_field_complete(Visitor *v, void *opaque) +{ + /* + * Do nothing, the complete method will be called in due time + * on the target visitor. + */ +} + +static void forward_field_free(Visitor *v) +{ + ForwardFieldVisitor *ffv = to_ffv(v); + + g_free(ffv->from); + g_free(ffv->to); + g_free(ffv); +} + +Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to) +{ + ForwardFieldVisitor *v = g_new0(ForwardFieldVisitor, 1); + + /* + * Clone and dealloc visitors don't use a name for the toplevel + * visit, so they make no sense here. + */ + assert(target->type == VISITOR_OUTPUT || target->type == VISITOR_INPUT); + + v->visitor.type = target->type; + v->visitor.start_struct = forward_field_start_struct; + v->visitor.check_struct = forward_field_check_struct; + v->visitor.end_struct = forward_field_end_struct; + v->visitor.start_list = forward_field_start_list; + v->visitor.next_list = forward_field_next_list; + v->visitor.check_list = forward_field_check_list; + v->visitor.end_list = forward_field_end_list; + v->visitor.start_alternate = forward_field_start_alternate; + v->visitor.end_alternate = forward_field_end_alternate; + v->visitor.type_int64 = forward_field_type_int64; + v->visitor.type_uint64 = forward_field_type_uint64; + v->visitor.type_size = forward_field_type_size; + v->visitor.type_bool = forward_field_type_bool; + v->visitor.type_str = forward_field_type_str; + v->visitor.type_number = forward_field_type_number; + v->visitor.type_any = forward_field_type_any; + v->visitor.type_null = forward_field_type_null; + v->visitor.optional = forward_field_optional; + v->visitor.deprecated_accept = forward_field_deprecated_accept; + v->visitor.deprecated = forward_field_deprecated; + v->visitor.complete = forward_field_complete; + v->visitor.free = forward_field_free; + + v->target = target; + v->from = g_strdup(from); + v->to = g_strdup(to); + + return &v->visitor; +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 3e0504dd21..5736d285b2 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -14,6 +14,7 @@ tests = { 'test-qobject-output-visitor': [testqapi], 'test-clone-visitor': [testqapi], 'test-qobject-input-visitor': [testqapi], + 'test-forward-visitor': [testqapi], 'test-string-input-visitor': [testqapi], 'test-string-output-visitor': [testqapi], 'test-opts-visitor': [testqapi], diff --git a/tests/unit/test-forward-visitor.c b/tests/unit/test-forward-visitor.c new file mode 100644 index 0000000000..348f7e4e81 --- /dev/null +++ b/tests/unit/test-forward-visitor.c @@ -0,0 +1,197 @@ +/* + * QAPI Forwarding Visitor unit-tests. + * + * Copyright (C) 2021 Red Hat Inc. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" + +#include "qemu-common.h" +#include "qapi/forward-visitor.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/error.h" +#include "qapi/qmp/qobject.h" +#include "qapi/qmp/qdict.h" +#include "test-qapi-visit.h" +#include "qemu/option.h" + +typedef bool GenericVisitor (Visitor *, const char *, void **, Error **); +#define CAST_VISIT_TYPE(fn) ((GenericVisitor *)(fn)) + +/* + * Parse @srcstr and wrap it with a ForwardFieldVisitor converting "src" to + * "dst". Check that visiting the result with "src" name fails, and return + * the result of visiting "dst". + */ +static void *visit_with_forward(const char *srcstr, GenericVisitor *fn) +{ + bool help = false; + QDict *src = keyval_parse(srcstr, NULL, &help, &error_abort); + Visitor *v, *alias_v; + Error *err = NULL; + void *result = NULL; + + v = qobject_input_visitor_new_keyval(QOBJECT(src)); + visit_start_struct(v, NULL, NULL, 0, &error_abort); + + alias_v = visitor_forward_field(v, "dst", "src"); + fn(alias_v, "src", &result, &err); + error_free_or_abort(&err); + assert(!result); + fn(alias_v, "dst", &result, &err); + assert(err == NULL); + visit_free(alias_v); + + visit_end_struct(v, NULL); + visit_free(v); + qobject_unref(QOBJECT(src)); + return result; +} + +static void test_forward_any(void) +{ + QObject *src = visit_with_forward("src.integer=42,src.string=Hello,src.enum1=value2", + CAST_VISIT_TYPE(visit_type_any)); + Visitor *v = qobject_input_visitor_new_keyval(src); + Error *err = NULL; + UserDefOne *dst; + + visit_type_UserDefOne(v, NULL, &dst, &err); + assert(err == NULL); + visit_free(v); + + g_assert_cmpint(dst->integer, ==, 42); + g_assert_cmpstr(dst->string, ==, "Hello"); + g_assert_cmpint(dst->has_enum1, ==, true); + g_assert_cmpint(dst->enum1, ==, ENUM_ONE_VALUE2); + qapi_free_UserDefOne(dst); + qobject_unref(QOBJECT(src)); +} + +static void test_forward_size(void) +{ + /* + * visit_type_size does not return a pointer, so visit_with_forward + * cannot be used. + */ + bool help = false; + QDict *src = keyval_parse("src=1.5M", NULL, &help, &error_abort); + Visitor *v, *alias_v; + Error *err = NULL; + uint64_t result = 0; + + v = qobject_input_visitor_new_keyval(QOBJECT(src)); + visit_start_struct(v, NULL, NULL, 0, &error_abort); + + alias_v = visitor_forward_field(v, "dst", "src"); + visit_type_size(alias_v, "src", &result, &err); + error_free_or_abort(&err); + visit_type_size(alias_v, "dst", &result, &err); + assert(result == 3 << 19); + assert(err == NULL); + visit_free(alias_v); + + visit_end_struct(v, NULL); + visit_free(v); + qobject_unref(QOBJECT(src)); +} + +static void test_forward_number(void) +{ + /* + * visit_type_number does not return a pointer, so visit_with_forward + * cannot be used. + */ + bool help = false; + QDict *src = keyval_parse("src=1.5", NULL, &help, &error_abort); + Visitor *v, *alias_v; + Error *err = NULL; + double result = 0.0; + + v = qobject_input_visitor_new_keyval(QOBJECT(src)); + visit_start_struct(v, NULL, NULL, 0, &error_abort); + + alias_v = visitor_forward_field(v, "dst", "src"); + visit_type_number(alias_v, "src", &result, &err); + error_free_or_abort(&err); + visit_type_number(alias_v, "dst", &result, &err); + assert(result == 1.5); + assert(err == NULL); + visit_free(alias_v); + + visit_end_struct(v, NULL); + visit_free(v); + qobject_unref(QOBJECT(src)); +} + +static void test_forward_string(void) +{ + char *dst = visit_with_forward("src=Hello", + CAST_VISIT_TYPE(visit_type_str)); + + g_assert_cmpstr(dst, ==, "Hello"); + g_free(dst); +} + +static void test_forward_struct(void) +{ + UserDefOne *dst = visit_with_forward("src.integer=42,src.string=Hello", + CAST_VISIT_TYPE(visit_type_UserDefOne)); + + g_assert_cmpint(dst->integer, ==, 42); + g_assert_cmpstr(dst->string, ==, "Hello"); + g_assert_cmpint(dst->has_enum1, ==, false); + qapi_free_UserDefOne(dst); +} + +static void test_forward_alternate(void) +{ + AltStrObj *s_dst = visit_with_forward("src=hello", + CAST_VISIT_TYPE(visit_type_AltStrObj)); + AltStrObj *o_dst = visit_with_forward("src.integer=42,src.boolean=true,src.string=world", + CAST_VISIT_TYPE(visit_type_AltStrObj)); + + g_assert_cmpint(s_dst->type, ==, QTYPE_QSTRING); + g_assert_cmpstr(s_dst->u.s, ==, "hello"); + g_assert_cmpint(o_dst->type, ==, QTYPE_QDICT); + g_assert_cmpint(o_dst->u.o.integer, ==, 42); + g_assert_cmpint(o_dst->u.o.boolean, ==, true); + g_assert_cmpstr(o_dst->u.o.string, ==, "world"); + + qapi_free_AltStrObj(s_dst); + qapi_free_AltStrObj(o_dst); +} + +static void test_forward_list(void) +{ + uint8List *dst = visit_with_forward("src.0=1,src.1=2,src.2=3,src.3=4", + CAST_VISIT_TYPE(visit_type_uint8List)); + uint8List *tmp; + int i; + + for (tmp = dst, i = 1; i <= 4; i++) { + g_assert(tmp); + g_assert_cmpint(tmp->value, ==, i); + tmp = tmp->next; + } + g_assert(!tmp); + qapi_free_uint8List(dst); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + g_test_add_func("/visitor/forward/struct", test_forward_struct); + g_test_add_func("/visitor/forward/alternate", test_forward_alternate); + g_test_add_func("/visitor/forward/string", test_forward_string); + g_test_add_func("/visitor/forward/size", test_forward_size); + g_test_add_func("/visitor/forward/number", test_forward_number); + g_test_add_func("/visitor/forward/any", test_forward_any); + g_test_add_func("/visitor/forward/list", test_forward_list); + + return g_test_run(); +} From cbc94d9702882128c52b72b252b8eb775b0e73af Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Sun, 18 Jul 2021 08:50:44 +0200 Subject: [PATCH 9/9] qom: use correct field name when getting/setting alias properties Alias targets have a different name than the alias property itself (e.g. a machine's pflash0 might be an alias of a property named 'drive'). When the target's getter or setter invokes the visitor, it will use a different name than what the caller expects, and the visitor will not be able to find it (or will consume erroneously). The solution is for alias getters and setters to wrap the incoming visitor, and forward the sole field that the target is expecting while renaming it appropriately. This bug has been there forever, but it was exposed after -M parsing switched from QemuOptions and StringInputVisitor to keyval and QObjectInputVisitor. Before, the visitor ignored the name. Now, it checks "drive" against what was passed on the command line and finds that no such property exists. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/484 Reported-by: Alex Williamson Signed-off-by: Paolo Bonzini --- qom/object.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/qom/object.c b/qom/object.c index 6a01d56546..e86cb05b84 100644 --- a/qom/object.c +++ b/qom/object.c @@ -20,6 +20,7 @@ #include "qapi/string-input-visitor.h" #include "qapi/string-output-visitor.h" #include "qapi/qobject-input-visitor.h" +#include "qapi/forward-visitor.h" #include "qapi/qapi-builtin-visit.h" #include "qapi/qmp/qerror.h" #include "qapi/qmp/qjson.h" @@ -2683,16 +2684,20 @@ static void property_get_alias(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { AliasProperty *prop = opaque; + Visitor *alias_v = visitor_forward_field(v, prop->target_name, name); - object_property_get(prop->target_obj, prop->target_name, v, errp); + object_property_get(prop->target_obj, prop->target_name, alias_v, errp); + visit_free(alias_v); } static void property_set_alias(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { AliasProperty *prop = opaque; + Visitor *alias_v = visitor_forward_field(v, prop->target_name, name); - object_property_set(prop->target_obj, prop->target_name, v, errp); + object_property_set(prop->target_obj, prop->target_name, alias_v, errp); + visit_free(alias_v); } static Object *property_resolve_alias(Object *obj, void *opaque,