* [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement
@ 2020-11-19 10:32 Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn() Vitaly Kuznetsov
` (6 more replies)
0 siblings, 7 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-11-19 10:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
This series is a part of the previously sent "[PATCH RFC v3 00/23] i386:
KVM: expand Hyper-V features early":
https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02443.html
We're not ready to merge the full patch set yet because the required
KVM capability is only queued for 5.11. We can, however, extract the
part providing 'hyperv=on' option to x86 machine types which is valuable
on its own. Picking up four other patches from the original RFC to
minimize the code churn in future (x86_cpu_realizefn()).
Changes since RFCv3:
- Rename 'hyperv_features' to 'default_hyperv_features' in X86MachineClass
[Eduardo]
- Move x86_cpu_hyperv_realize() invocation after x86_cpu_expand_features()/
x86_cpu_filter_features() as we need to reference cpu_has_vmx().
Vitaly Kuznetsov (5):
i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
i386: move hyperv_interface_id initialization to x86_cpu_realizefn()
i386: move hyperv_version_id initialization to x86_cpu_realizefn()
i386: move hyperv_limits initialization to x86_cpu_realizefn()
i386: provide simple 'hyperv=on' option to x86 machine types
docs/hyperv.txt | 8 +++++
hw/i386/x86.c | 30 +++++++++++++++++++
include/hw/i386/x86.h | 7 +++++
target/i386/cpu.c | 52 +++++++++++++++++++++++++++++++-
target/i386/cpu.h | 6 +++-
target/i386/kvm.c | 70 ++++++++++++++++++++++++++++---------------
6 files changed, 147 insertions(+), 26 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
@ 2020-11-19 10:32 ` Vitaly Kuznetsov
2021-03-10 11:27 ` Claudio Fontana
2020-11-19 10:32 ` [PATCH 2/5] i386: move hyperv_interface_id " Vitaly Kuznetsov
` (5 subsequent siblings)
6 siblings, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-11-19 10:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
As a preparation to expanding Hyper-V CPU features early, move
hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
itself.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
target/i386/cpu.c | 23 ++++++++++++++++++++++-
target/i386/cpu.h | 3 ++-
target/i386/kvm.c | 25 ++++++++++---------------
3 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5a8c96072e41..2a6885753378 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6509,6 +6509,25 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
}
}
+static void x86_cpu_hyperv_realize(X86CPU *cpu)
+{
+ size_t len;
+
+ /* Hyper-V vendor id */
+ if (!cpu->hyperv_vendor) {
+ memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
+ } else {
+ len = strlen(cpu->hyperv_vendor);
+
+ if (len > 12) {
+ warn_report("hv-vendor-id truncated to 12 characters");
+ len = 12;
+ }
+ memset(cpu->hyperv_vendor_id, 0, 12);
+ memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
+ }
+}
+
static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
{
CPUState *cs = CPU(dev);
@@ -6680,6 +6699,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
env->cache_info_amd.l3_cache = &legacy_l3_cache;
}
+ /* Process Hyper-V enlightenments */
+ x86_cpu_hyperv_realize(cpu);
cpu_exec_realizefn(cs, &local_err);
if (local_err != NULL) {
@@ -7198,7 +7219,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
- DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
+ DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 88e8586f8fb4..be640bf45c29 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1655,11 +1655,12 @@ struct X86CPU {
uint64_t ucode_rev;
uint32_t hyperv_spinlock_attempts;
- char *hyperv_vendor_id;
+ char *hyperv_vendor;
bool hyperv_synic_kvm_only;
uint64_t hyperv_features;
bool hyperv_passthrough;
OnOffAuto hyperv_no_nonarch_cs;
+ uint32_t hyperv_vendor_id[3];
bool check_cpuid;
bool enforce_cpuid;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index a2934dda027c..788a2cf2ec51 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1205,6 +1205,13 @@ static int hyperv_handle_properties(CPUState *cs,
memcpy(cpuid_ent, &cpuid->entries[0],
cpuid->nent * sizeof(cpuid->entries[0]));
+ c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
+ if (c) {
+ cpu->hyperv_vendor_id[0] = c->ebx;
+ cpu->hyperv_vendor_id[1] = c->ecx;
+ cpu->hyperv_vendor_id[2] = c->edx;
+ }
+
c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
if (c) {
env->features[FEAT_HYPERV_EAX] = c->eax;
@@ -1279,23 +1286,11 @@ static int hyperv_handle_properties(CPUState *cs,
c = &cpuid_ent[cpuid_i++];
c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
- if (!cpu->hyperv_vendor_id) {
- memcpy(signature, "Microsoft Hv", 12);
- } else {
- size_t len = strlen(cpu->hyperv_vendor_id);
-
- if (len > 12) {
- error_report("hv-vendor-id truncated to 12 characters");
- len = 12;
- }
- memset(signature, 0, 12);
- memcpy(signature, cpu->hyperv_vendor_id, len);
- }
c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
- c->ebx = signature[0];
- c->ecx = signature[1];
- c->edx = signature[2];
+ c->ebx = cpu->hyperv_vendor_id[0];
+ c->ecx = cpu->hyperv_vendor_id[1];
+ c->edx = cpu->hyperv_vendor_id[2];
c = &cpuid_ent[cpuid_i++];
c->function = HV_CPUID_INTERFACE;
--
2.26.2
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/5] i386: move hyperv_interface_id initialization to x86_cpu_realizefn()
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn() Vitaly Kuznetsov
@ 2020-11-19 10:32 ` Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 3/5] i386: move hyperv_version_id " Vitaly Kuznetsov
` (4 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-11-19 10:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
As a preparation to expanding Hyper-V CPU features early, move
hyperv_interface_id initialization to x86_cpu_realizefn().
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
target/i386/cpu.c | 6 ++++++
target/i386/cpu.h | 1 +
target/i386/kvm.c | 18 ++++++++++++------
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2a6885753378..18a73ca4708b 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6526,6 +6526,12 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
memset(cpu->hyperv_vendor_id, 0, 12);
memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
}
+
+ /* 'Hv#1' interface identification*/
+ cpu->hyperv_interface_id[0] = 0x31237648;
+ cpu->hyperv_interface_id[1] = 0;
+ cpu->hyperv_interface_id[2] = 0;
+ cpu->hyperv_interface_id[3] = 0;
}
static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index be640bf45c29..1f30d642007c 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1661,6 +1661,7 @@ struct X86CPU {
bool hyperv_passthrough;
OnOffAuto hyperv_no_nonarch_cs;
uint32_t hyperv_vendor_id[3];
+ uint32_t hyperv_interface_id[4];
bool check_cpuid;
bool enforce_cpuid;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 788a2cf2ec51..eb09766ec9b7 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1168,7 +1168,6 @@ static int hyperv_handle_properties(CPUState *cs,
CPUX86State *env = &cpu->env;
struct kvm_cpuid2 *cpuid;
struct kvm_cpuid_entry2 *c;
- uint32_t signature[3];
uint32_t cpuid_i = 0;
int r;
@@ -1212,6 +1211,14 @@ static int hyperv_handle_properties(CPUState *cs,
cpu->hyperv_vendor_id[2] = c->edx;
}
+ c = cpuid_find_entry(cpuid, HV_CPUID_INTERFACE, 0);
+ if (c) {
+ cpu->hyperv_interface_id[0] = c->eax;
+ cpu->hyperv_interface_id[1] = c->ebx;
+ cpu->hyperv_interface_id[2] = c->ecx;
+ cpu->hyperv_interface_id[3] = c->edx;
+ }
+
c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
if (c) {
env->features[FEAT_HYPERV_EAX] = c->eax;
@@ -1294,11 +1301,10 @@ static int hyperv_handle_properties(CPUState *cs,
c = &cpuid_ent[cpuid_i++];
c->function = HV_CPUID_INTERFACE;
- memcpy(signature, "Hv#1\0\0\0\0\0\0\0\0", 12);
- c->eax = signature[0];
- c->ebx = 0;
- c->ecx = 0;
- c->edx = 0;
+ c->eax = cpu->hyperv_interface_id[0];
+ c->ebx = cpu->hyperv_interface_id[1];
+ c->ecx = cpu->hyperv_interface_id[2];
+ c->edx = cpu->hyperv_interface_id[3];
c = &cpuid_ent[cpuid_i++];
c->function = HV_CPUID_VERSION;
--
2.26.2
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/5] i386: move hyperv_version_id initialization to x86_cpu_realizefn()
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn() Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 2/5] i386: move hyperv_interface_id " Vitaly Kuznetsov
@ 2020-11-19 10:32 ` Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 4/5] i386: move hyperv_limits " Vitaly Kuznetsov
` (3 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-11-19 10:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
As a preparation to expanding Hyper-V CPU features early, move
hyperv_version_id initialization to x86_cpu_realizefn().
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
target/i386/cpu.c | 4 ++++
target/i386/cpu.h | 1 +
target/i386/kvm.c | 14 ++++++++++++--
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 18a73ca4708b..9e182929d29d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6532,6 +6532,10 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
cpu->hyperv_interface_id[1] = 0;
cpu->hyperv_interface_id[2] = 0;
cpu->hyperv_interface_id[3] = 0;
+
+ /* Hypervisor system identity */
+ cpu->hyperv_version_id[0] = 0x00001bbc;
+ cpu->hyperv_version_id[1] = 0x00060001;
}
static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 1f30d642007c..913927356c55 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1662,6 +1662,7 @@ struct X86CPU {
OnOffAuto hyperv_no_nonarch_cs;
uint32_t hyperv_vendor_id[3];
uint32_t hyperv_interface_id[4];
+ uint32_t hyperv_version_id[4];
bool check_cpuid;
bool enforce_cpuid;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index eb09766ec9b7..58a2528c6c41 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1219,6 +1219,14 @@ static int hyperv_handle_properties(CPUState *cs,
cpu->hyperv_interface_id[3] = c->edx;
}
+ c = cpuid_find_entry(cpuid, HV_CPUID_VERSION, 0);
+ if (c) {
+ cpu->hyperv_version_id[0] = c->eax;
+ cpu->hyperv_version_id[1] = c->ebx;
+ cpu->hyperv_version_id[2] = c->ecx;
+ cpu->hyperv_version_id[3] = c->edx;
+ }
+
c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
if (c) {
env->features[FEAT_HYPERV_EAX] = c->eax;
@@ -1308,8 +1316,10 @@ static int hyperv_handle_properties(CPUState *cs,
c = &cpuid_ent[cpuid_i++];
c->function = HV_CPUID_VERSION;
- c->eax = 0x00001bbc;
- c->ebx = 0x00060001;
+ c->eax = cpu->hyperv_version_id[0];
+ c->ebx = cpu->hyperv_version_id[1];
+ c->ecx = cpu->hyperv_version_id[2];
+ c->edx = cpu->hyperv_version_id[3];
c = &cpuid_ent[cpuid_i++];
c->function = HV_CPUID_FEATURES;
--
2.26.2
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 4/5] i386: move hyperv_limits initialization to x86_cpu_realizefn()
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
` (2 preceding siblings ...)
2020-11-19 10:32 ` [PATCH 3/5] i386: move hyperv_version_id " Vitaly Kuznetsov
@ 2020-11-19 10:32 ` Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types Vitaly Kuznetsov
` (2 subsequent siblings)
6 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-11-19 10:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
As a preparation to expanding Hyper-V CPU features early, move
hyperv_limits initialization to x86_cpu_realizefn().
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
target/i386/cpu.c | 5 +++++
target/i386/cpu.h | 1 +
target/i386/kvm.c | 13 ++++++++++++-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9e182929d29d..83aca942d87c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6536,6 +6536,11 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
/* Hypervisor system identity */
cpu->hyperv_version_id[0] = 0x00001bbc;
cpu->hyperv_version_id[1] = 0x00060001;
+
+ /* Hypervisor implementation limits */
+ cpu->hyperv_limits[0] = 64;
+ cpu->hyperv_limits[1] = 0;
+ cpu->hyperv_limits[2] = 0;
}
static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 913927356c55..c95f20f59b15 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1663,6 +1663,7 @@ struct X86CPU {
uint32_t hyperv_vendor_id[3];
uint32_t hyperv_interface_id[4];
uint32_t hyperv_version_id[4];
+ uint32_t hyperv_limits[3];
bool check_cpuid;
bool enforce_cpuid;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 58a2528c6c41..446ab2ef1793 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1233,6 +1233,15 @@ static int hyperv_handle_properties(CPUState *cs,
env->features[FEAT_HYPERV_EBX] = c->ebx;
env->features[FEAT_HYPERV_EDX] = c->edx;
}
+
+ c = cpuid_find_entry(cpuid, HV_CPUID_IMPLEMENT_LIMITS, 0);
+ if (c) {
+ cpu->hv_max_vps = c->eax;
+ cpu->hyperv_limits[0] = c->ebx;
+ cpu->hyperv_limits[1] = c->ecx;
+ cpu->hyperv_limits[2] = c->edx;
+ }
+
c = cpuid_find_entry(cpuid, HV_CPUID_ENLIGHTMENT_INFO, 0);
if (c) {
env->features[FEAT_HV_RECOMM_EAX] = c->eax;
@@ -1335,7 +1344,9 @@ static int hyperv_handle_properties(CPUState *cs,
c = &cpuid_ent[cpuid_i++];
c->function = HV_CPUID_IMPLEMENT_LIMITS;
c->eax = cpu->hv_max_vps;
- c->ebx = 0x40;
+ c->ebx = cpu->hyperv_limits[0];
+ c->ecx = cpu->hyperv_limits[1];
+ c->edx = cpu->hyperv_limits[2];
if (hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS)) {
__u32 function;
--
2.26.2
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
` (3 preceding siblings ...)
2020-11-19 10:32 ` [PATCH 4/5] i386: move hyperv_limits " Vitaly Kuznetsov
@ 2020-11-19 10:32 ` Vitaly Kuznetsov
2020-12-16 20:52 ` Eduardo Habkost
2020-11-19 14:22 ` [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Claudio Fontana
2020-12-16 19:09 ` Eduardo Habkost
6 siblings, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-11-19 10:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
requires listing all currently supported enlightenments ("hv_*" CPU
features) explicitly. We do have a 'hv_passthrough' mode enabling
everything but it can't be used in production as it prevents migration.
Introduce a simple 'hyperv=on' option for all x86 machine types enabling
all currently supported Hyper-V enlightenments. Later, when new
enlightenments get implemented, we will be adding them to newer machine
types only (by disabling them for legacy machine types) thus preserving
migration.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
docs/hyperv.txt | 8 ++++++++
hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++
include/hw/i386/x86.h | 7 +++++++
target/i386/cpu.c | 14 ++++++++++++++
4 files changed, 59 insertions(+)
diff --git a/docs/hyperv.txt b/docs/hyperv.txt
index 5df00da54fc4..1a76a07f8417 100644
--- a/docs/hyperv.txt
+++ b/docs/hyperv.txt
@@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor
identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification
and features are kept in leaves 0x40000100..0x40000101.
+Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
+x86 machine type:
+
+ qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ...
+
+Note, new enlightenments are only added to the latest (in-develompent) machine
+type, older machine types keep the list of the supported features intact to
+safeguard migration.
3. Existing enlightenments
===========================
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 5944fc44edca..57f27d56ecc6 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name,
visit_type_OnOffAuto(v, name, &x86ms->acpi, errp);
}
+static bool x86_machine_get_hyperv(Object *obj, Error **errp)
+{
+ X86MachineState *x86ms = X86_MACHINE(obj);
+
+ return x86ms->hyperv_enabled;
+}
+
+static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
+{
+ X86MachineState *x86ms = X86_MACHINE(obj);
+
+ x86ms->hyperv_enabled = value;
+}
+
static void x86_machine_initfn(Object *obj)
{
X86MachineState *x86ms = X86_MACHINE(obj);
@@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
x86mc->save_tsc_khz = true;
nc->nmi_monitor_handler = x86_nmi;
+ /* Hyper-V features enabled with 'hyperv=on' */
+ x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
+ BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
+ BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
+ BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
+ BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
+ BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
+ BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
+ BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
+
object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
x86_machine_get_smm, x86_machine_set_smm,
NULL, NULL);
@@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
NULL, NULL);
object_class_property_set_description(oc, X86_MACHINE_ACPI,
"Enable ACPI");
+
+ object_class_property_add_bool(oc, X86_MACHINE_HYPERV,
+ x86_machine_get_hyperv, x86_machine_set_hyperv);
+
+ object_class_property_set_description(oc, X86_MACHINE_HYPERV,
+ "Enable Hyper-V enlightenments");
}
static const TypeInfo x86_machine_info = {
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 739fac50871b..598abd1be806 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -38,6 +38,9 @@ struct X86MachineClass {
bool save_tsc_khz;
/* Enables contiguous-apic-ID mode */
bool compat_apic_id_mode;
+
+ /* Hyper-V features enabled with 'hyperv=on' */
+ uint64_t default_hyperv_features;
};
struct X86MachineState {
@@ -71,10 +74,14 @@ struct X86MachineState {
* will be translated to MSI messages in the address space.
*/
AddressSpace *ioapic_as;
+
+ /* Hyper-V emulation */
+ bool hyperv_enabled;
};
#define X86_MACHINE_SMM "smm"
#define X86_MACHINE_ACPI "acpi"
+#define X86_MACHINE_HYPERV "hyperv"
#define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86")
OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 83aca942d87c..63a931679d73 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -53,6 +53,7 @@
#include "sysemu/tcg.h"
#include "hw/qdev-properties.h"
#include "hw/i386/topology.h"
+#include "hw/i386/x86.h"
#ifndef CONFIG_USER_ONLY
#include "exec/address-spaces.h"
#include "hw/i386/apic_internal.h"
@@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
static void x86_cpu_hyperv_realize(X86CPU *cpu)
{
+ X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
+ X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
+ uint64_t feat;
size_t len;
+ if (x86ms->hyperv_enabled) {
+ feat = x86mc->default_hyperv_features;
+ /* Enlightened VMCS is only available on Intel/VMX */
+ if (!cpu_has_vmx(&cpu->env)) {
+ feat &= ~BIT(HYPERV_FEAT_EVMCS);
+ }
+
+ cpu->hyperv_features |= feat;
+ }
+
/* Hyper-V vendor id */
if (!cpu->hyperv_vendor) {
memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
--
2.26.2
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
` (4 preceding siblings ...)
2020-11-19 10:32 ` [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types Vitaly Kuznetsov
@ 2020-11-19 14:22 ` Claudio Fontana
2020-11-19 16:58 ` Vitaly Kuznetsov
2020-12-16 19:09 ` Eduardo Habkost
6 siblings, 1 reply; 39+ messages in thread
From: Claudio Fontana @ 2020-11-19 14:22 UTC (permalink / raw)
To: Vitaly Kuznetsov, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
Hi Vitaly, I just wanted to raise awareness of
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg04597.html
because if that series work is completed, you would have already the right hook to put your code in, when it comes to your hyperv-specific code for the realizefn.
Ciao ciao,
Claudio
On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
> This series is a part of the previously sent "[PATCH RFC v3 00/23] i386:
> KVM: expand Hyper-V features early":
> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02443.html
>
> We're not ready to merge the full patch set yet because the required
> KVM capability is only queued for 5.11. We can, however, extract the
> part providing 'hyperv=on' option to x86 machine types which is valuable
> on its own. Picking up four other patches from the original RFC to
> minimize the code churn in future (x86_cpu_realizefn()).
>
> Changes since RFCv3:
> - Rename 'hyperv_features' to 'default_hyperv_features' in X86MachineClass
> [Eduardo]
> - Move x86_cpu_hyperv_realize() invocation after x86_cpu_expand_features()/
> x86_cpu_filter_features() as we need to reference cpu_has_vmx().
>
> Vitaly Kuznetsov (5):
> i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
> i386: move hyperv_interface_id initialization to x86_cpu_realizefn()
> i386: move hyperv_version_id initialization to x86_cpu_realizefn()
> i386: move hyperv_limits initialization to x86_cpu_realizefn()
> i386: provide simple 'hyperv=on' option to x86 machine types
>
> docs/hyperv.txt | 8 +++++
> hw/i386/x86.c | 30 +++++++++++++++++++
> include/hw/i386/x86.h | 7 +++++
> target/i386/cpu.c | 52 +++++++++++++++++++++++++++++++-
> target/i386/cpu.h | 6 +++-
> target/i386/kvm.c | 70 ++++++++++++++++++++++++++++---------------
> 6 files changed, 147 insertions(+), 26 deletions(-)
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement
2020-11-19 14:22 ` [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Claudio Fontana
@ 2020-11-19 16:58 ` Vitaly Kuznetsov
0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-11-19 16:58 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
Claudio Fontana <cfontana@suse.de> writes:
> Hi Vitaly, I just wanted to raise awareness of
>
> https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg04597.html
>
> because if that series work is completed, you would have already the
> right hook to put your code in, when it comes to your hyperv-specific
> code for the realizefn.
Hi Claudio,
thanks for letting me know! I'll take a look but at first glance it
would just be a code movement. Hope your series gets merged soon, at
least before my bug 'i386: KVM: expand Hyper-V features early"' is
coming (will be submitting it after 5.11-rc1 kernel upstream) so we
minimize the code churn.
>
> Ciao ciao,
>
> Claudio
>
> On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
>> This series is a part of the previously sent "[PATCH RFC v3 00/23] i386:
>> KVM: expand Hyper-V features early":
>> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02443.html
>>
>> We're not ready to merge the full patch set yet because the required
>> KVM capability is only queued for 5.11. We can, however, extract the
>> part providing 'hyperv=on' option to x86 machine types which is valuable
>> on its own. Picking up four other patches from the original RFC to
>> minimize the code churn in future (x86_cpu_realizefn()).
>>
>> Changes since RFCv3:
>> - Rename 'hyperv_features' to 'default_hyperv_features' in X86MachineClass
>> [Eduardo]
>> - Move x86_cpu_hyperv_realize() invocation after x86_cpu_expand_features()/
>> x86_cpu_filter_features() as we need to reference cpu_has_vmx().
>>
>> Vitaly Kuznetsov (5):
>> i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
>> i386: move hyperv_interface_id initialization to x86_cpu_realizefn()
>> i386: move hyperv_version_id initialization to x86_cpu_realizefn()
>> i386: move hyperv_limits initialization to x86_cpu_realizefn()
>> i386: provide simple 'hyperv=on' option to x86 machine types
>>
>> docs/hyperv.txt | 8 +++++
>> hw/i386/x86.c | 30 +++++++++++++++++++
>> include/hw/i386/x86.h | 7 +++++
>> target/i386/cpu.c | 52 +++++++++++++++++++++++++++++++-
>> target/i386/cpu.h | 6 +++-
>> target/i386/kvm.c | 70 ++++++++++++++++++++++++++++---------------
>> 6 files changed, 147 insertions(+), 26 deletions(-)
>>
>
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
` (5 preceding siblings ...)
2020-11-19 14:22 ` [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Claudio Fontana
@ 2020-12-16 19:09 ` Eduardo Habkost
6 siblings, 0 replies; 39+ messages in thread
From: Eduardo Habkost @ 2020-12-16 19:09 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel
On Thu, Nov 19, 2020 at 11:32:16AM +0100, Vitaly Kuznetsov wrote:
> This series is a part of the previously sent "[PATCH RFC v3 00/23] i386:
> KVM: expand Hyper-V features early":
> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02443.html
>
> We're not ready to merge the full patch set yet because the required
> KVM capability is only queued for 5.11. We can, however, extract the
> part providing 'hyperv=on' option to x86 machine types which is valuable
> on its own. Picking up four other patches from the original RFC to
> minimize the code churn in future (x86_cpu_realizefn()).
>
> Changes since RFCv3:
> - Rename 'hyperv_features' to 'default_hyperv_features' in X86MachineClass
> [Eduardo]
> - Move x86_cpu_hyperv_realize() invocation after x86_cpu_expand_features()/
> x86_cpu_filter_features() as we need to reference cpu_has_vmx().
>
> Vitaly Kuznetsov (5):
> i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
> i386: move hyperv_interface_id initialization to x86_cpu_realizefn()
> i386: move hyperv_version_id initialization to x86_cpu_realizefn()
> i386: move hyperv_limits initialization to x86_cpu_realizefn()
> i386: provide simple 'hyperv=on' option to x86 machine types
Queued, thanks!
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-11-19 10:32 ` [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types Vitaly Kuznetsov
@ 2020-12-16 20:52 ` Eduardo Habkost
2020-12-17 9:34 ` Vitaly Kuznetsov
2020-12-18 17:13 ` Igor Mammedov
0 siblings, 2 replies; 39+ messages in thread
From: Eduardo Habkost @ 2020-12-16 20:52 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel
On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
> Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
> requires listing all currently supported enlightenments ("hv_*" CPU
> features) explicitly. We do have a 'hv_passthrough' mode enabling
> everything but it can't be used in production as it prevents migration.
>
> Introduce a simple 'hyperv=on' option for all x86 machine types enabling
> all currently supported Hyper-V enlightenments. Later, when new
> enlightenments get implemented, we will be adding them to newer machine
> types only (by disabling them for legacy machine types) thus preserving
> migration.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
[...]
> ---
> docs/hyperv.txt | 8 ++++++++
> hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++
> include/hw/i386/x86.h | 7 +++++++
> target/i386/cpu.c | 14 ++++++++++++++
> 4 files changed, 59 insertions(+)
>
> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> index 5df00da54fc4..1a76a07f8417 100644
> --- a/docs/hyperv.txt
> +++ b/docs/hyperv.txt
> @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor
> identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification
> and features are kept in leaves 0x40000100..0x40000101.
>
> +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
> +x86 machine type:
> +
> + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ...
> +
> +Note, new enlightenments are only added to the latest (in-develompent) machine
> +type, older machine types keep the list of the supported features intact to
> +safeguard migration.
>
> 3. Existing enlightenments
> ===========================
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 5944fc44edca..57f27d56ecc6 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name,
> visit_type_OnOffAuto(v, name, &x86ms->acpi, errp);
> }
>
> +static bool x86_machine_get_hyperv(Object *obj, Error **errp)
> +{
> + X86MachineState *x86ms = X86_MACHINE(obj);
> +
> + return x86ms->hyperv_enabled;
> +}
> +
> +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
> +{
> + X86MachineState *x86ms = X86_MACHINE(obj);
> +
> + x86ms->hyperv_enabled = value;
> +}
> +
> static void x86_machine_initfn(Object *obj)
> {
> X86MachineState *x86ms = X86_MACHINE(obj);
> @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
> x86mc->save_tsc_khz = true;
> nc->nmi_monitor_handler = x86_nmi;
>
> + /* Hyper-V features enabled with 'hyperv=on' */
> + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> +
> object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
> x86_machine_get_smm, x86_machine_set_smm,
> NULL, NULL);
> @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
> NULL, NULL);
> object_class_property_set_description(oc, X86_MACHINE_ACPI,
> "Enable ACPI");
> +
> + object_class_property_add_bool(oc, X86_MACHINE_HYPERV,
> + x86_machine_get_hyperv, x86_machine_set_hyperv);
> +
> + object_class_property_set_description(oc, X86_MACHINE_HYPERV,
> + "Enable Hyper-V enlightenments");
> }
>
> static const TypeInfo x86_machine_info = {
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index 739fac50871b..598abd1be806 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -38,6 +38,9 @@ struct X86MachineClass {
> bool save_tsc_khz;
> /* Enables contiguous-apic-ID mode */
> bool compat_apic_id_mode;
> +
> + /* Hyper-V features enabled with 'hyperv=on' */
> + uint64_t default_hyperv_features;
> };
>
> struct X86MachineState {
> @@ -71,10 +74,14 @@ struct X86MachineState {
> * will be translated to MSI messages in the address space.
> */
> AddressSpace *ioapic_as;
> +
> + /* Hyper-V emulation */
> + bool hyperv_enabled;
> };
>
> #define X86_MACHINE_SMM "smm"
> #define X86_MACHINE_ACPI "acpi"
> +#define X86_MACHINE_HYPERV "hyperv"
>
> #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86")
> OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 83aca942d87c..63a931679d73 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -53,6 +53,7 @@
> #include "sysemu/tcg.h"
> #include "hw/qdev-properties.h"
> #include "hw/i386/topology.h"
> +#include "hw/i386/x86.h"
> #ifndef CONFIG_USER_ONLY
> #include "exec/address-spaces.h"
> #include "hw/i386/apic_internal.h"
> @@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>
> static void x86_cpu_hyperv_realize(X86CPU *cpu)
> {
> + X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> + X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> + uint64_t feat;
> size_t len;
>
> + if (x86ms->hyperv_enabled) {
> + feat = x86mc->default_hyperv_features;
> + /* Enlightened VMCS is only available on Intel/VMX */
> + if (!cpu_has_vmx(&cpu->env)) {
> + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> + }
> +
> + cpu->hyperv_features |= feat;
> + }
I had to dequeue this because it doesn't compile with
CONFIG_USER_ONLY:
https://gitlab.com/ehabkost/qemu/-/jobs/916651017
The easiest solution would be to wrap the new code in #ifndef
CONFIG_USER_ONLY, but maybe we should try to move all
X86Machine-specific code from cpu.c to
hw/i386/x86.c:x86_cpu_pre_plug().
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-12-16 20:52 ` Eduardo Habkost
@ 2020-12-17 9:34 ` Vitaly Kuznetsov
2020-12-18 17:13 ` Igor Mammedov
1 sibling, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-12-17 9:34 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
>> Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
>> requires listing all currently supported enlightenments ("hv_*" CPU
>> features) explicitly. We do have a 'hv_passthrough' mode enabling
>> everything but it can't be used in production as it prevents migration.
>>
>> Introduce a simple 'hyperv=on' option for all x86 machine types enabling
>> all currently supported Hyper-V enlightenments. Later, when new
>> enlightenments get implemented, we will be adding them to newer machine
>> types only (by disabling them for legacy machine types) thus preserving
>> migration.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> [...]
>> ---
>> docs/hyperv.txt | 8 ++++++++
>> hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++
>> include/hw/i386/x86.h | 7 +++++++
>> target/i386/cpu.c | 14 ++++++++++++++
>> 4 files changed, 59 insertions(+)
>>
>> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
>> index 5df00da54fc4..1a76a07f8417 100644
>> --- a/docs/hyperv.txt
>> +++ b/docs/hyperv.txt
>> @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor
>> identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification
>> and features are kept in leaves 0x40000100..0x40000101.
>>
>> +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
>> +x86 machine type:
>> +
>> + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ...
>> +
>> +Note, new enlightenments are only added to the latest (in-develompent) machine
>> +type, older machine types keep the list of the supported features intact to
>> +safeguard migration.
>>
>> 3. Existing enlightenments
>> ===========================
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index 5944fc44edca..57f27d56ecc6 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name,
>> visit_type_OnOffAuto(v, name, &x86ms->acpi, errp);
>> }
>>
>> +static bool x86_machine_get_hyperv(Object *obj, Error **errp)
>> +{
>> + X86MachineState *x86ms = X86_MACHINE(obj);
>> +
>> + return x86ms->hyperv_enabled;
>> +}
>> +
>> +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
>> +{
>> + X86MachineState *x86ms = X86_MACHINE(obj);
>> +
>> + x86ms->hyperv_enabled = value;
>> +}
>> +
>> static void x86_machine_initfn(Object *obj)
>> {
>> X86MachineState *x86ms = X86_MACHINE(obj);
>> @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
>> x86mc->save_tsc_khz = true;
>> nc->nmi_monitor_handler = x86_nmi;
>>
>> + /* Hyper-V features enabled with 'hyperv=on' */
>> + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
>> + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
>> + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
>> + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
>> + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
>> + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
>> + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
>> + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
>> +
>> object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
>> x86_machine_get_smm, x86_machine_set_smm,
>> NULL, NULL);
>> @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
>> NULL, NULL);
>> object_class_property_set_description(oc, X86_MACHINE_ACPI,
>> "Enable ACPI");
>> +
>> + object_class_property_add_bool(oc, X86_MACHINE_HYPERV,
>> + x86_machine_get_hyperv, x86_machine_set_hyperv);
>> +
>> + object_class_property_set_description(oc, X86_MACHINE_HYPERV,
>> + "Enable Hyper-V enlightenments");
>> }
>>
>> static const TypeInfo x86_machine_info = {
>> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
>> index 739fac50871b..598abd1be806 100644
>> --- a/include/hw/i386/x86.h
>> +++ b/include/hw/i386/x86.h
>> @@ -38,6 +38,9 @@ struct X86MachineClass {
>> bool save_tsc_khz;
>> /* Enables contiguous-apic-ID mode */
>> bool compat_apic_id_mode;
>> +
>> + /* Hyper-V features enabled with 'hyperv=on' */
>> + uint64_t default_hyperv_features;
>> };
>>
>> struct X86MachineState {
>> @@ -71,10 +74,14 @@ struct X86MachineState {
>> * will be translated to MSI messages in the address space.
>> */
>> AddressSpace *ioapic_as;
>> +
>> + /* Hyper-V emulation */
>> + bool hyperv_enabled;
>> };
>>
>> #define X86_MACHINE_SMM "smm"
>> #define X86_MACHINE_ACPI "acpi"
>> +#define X86_MACHINE_HYPERV "hyperv"
>>
>> #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86")
>> OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 83aca942d87c..63a931679d73 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -53,6 +53,7 @@
>> #include "sysemu/tcg.h"
>> #include "hw/qdev-properties.h"
>> #include "hw/i386/topology.h"
>> +#include "hw/i386/x86.h"
>> #ifndef CONFIG_USER_ONLY
>> #include "exec/address-spaces.h"
>> #include "hw/i386/apic_internal.h"
>> @@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>>
>> static void x86_cpu_hyperv_realize(X86CPU *cpu)
>> {
>> + X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
>> + X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
>> + uint64_t feat;
>> size_t len;
>>
>> + if (x86ms->hyperv_enabled) {
>> + feat = x86mc->default_hyperv_features;
>> + /* Enlightened VMCS is only available on Intel/VMX */
>> + if (!cpu_has_vmx(&cpu->env)) {
>> + feat &= ~BIT(HYPERV_FEAT_EVMCS);
>> + }
>> +
>> + cpu->hyperv_features |= feat;
>> + }
>
> I had to dequeue this because it doesn't compile with
> CONFIG_USER_ONLY:
>
> https://gitlab.com/ehabkost/qemu/-/jobs/916651017
>
> The easiest solution would be to wrap the new code in #ifndef
> CONFIG_USER_ONLY, but maybe we should try to move all
> X86Machine-specific code from cpu.c to
> hw/i386/x86.c:x86_cpu_pre_plug().
Bummer,
let me try the suggestion.
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-12-16 20:52 ` Eduardo Habkost
2020-12-17 9:34 ` Vitaly Kuznetsov
@ 2020-12-18 17:13 ` Igor Mammedov
2020-12-18 18:07 ` Eduardo Habkost
2021-01-04 12:54 ` Vitaly Kuznetsov
1 sibling, 2 replies; 39+ messages in thread
From: Igor Mammedov @ 2020-12-18 17:13 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Wed, 16 Dec 2020 15:52:02 -0500
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
> > Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
> > requires listing all currently supported enlightenments ("hv_*" CPU
> > features) explicitly. We do have a 'hv_passthrough' mode enabling
> > everything but it can't be used in production as it prevents migration.
> >
> > Introduce a simple 'hyperv=on' option for all x86 machine types enabling
> > all currently supported Hyper-V enlightenments. Later, when new
> > enlightenments get implemented, we will be adding them to newer machine
> > types only (by disabling them for legacy machine types) thus preserving
> > migration.
> >
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> [...]
> > ---
> > docs/hyperv.txt | 8 ++++++++
> > hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++
> > include/hw/i386/x86.h | 7 +++++++
> > target/i386/cpu.c | 14 ++++++++++++++
> > 4 files changed, 59 insertions(+)
> >
> > diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > index 5df00da54fc4..1a76a07f8417 100644
> > --- a/docs/hyperv.txt
> > +++ b/docs/hyperv.txt
> > @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor
> > identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification
> > and features are kept in leaves 0x40000100..0x40000101.
> >
> > +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
> > +x86 machine type:
> > +
> > + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ...
> > +
> > +Note, new enlightenments are only added to the latest (in-develompent) machine
> > +type, older machine types keep the list of the supported features intact to
> > +safeguard migration.
> >
> > 3. Existing enlightenments
> > ===========================
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > index 5944fc44edca..57f27d56ecc6 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name,
> > visit_type_OnOffAuto(v, name, &x86ms->acpi, errp);
> > }
> >
> > +static bool x86_machine_get_hyperv(Object *obj, Error **errp)
> > +{
> > + X86MachineState *x86ms = X86_MACHINE(obj);
> > +
> > + return x86ms->hyperv_enabled;
> > +}
> > +
> > +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
> > +{
> > + X86MachineState *x86ms = X86_MACHINE(obj);
> > +
> > + x86ms->hyperv_enabled = value;
> > +}
> > +
> > static void x86_machine_initfn(Object *obj)
> > {
> > X86MachineState *x86ms = X86_MACHINE(obj);
> > @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
> > x86mc->save_tsc_khz = true;
> > nc->nmi_monitor_handler = x86_nmi;
> >
> > + /* Hyper-V features enabled with 'hyperv=on' */
> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
I'd argue that feature bits do not belong to machine code at all.
If we have to involve machine at all then it should be a set property/value pairs
that machine will set on CPU object (I'm not convinced that doing it
from machine code is good idea though).
> > +
> > object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
> > x86_machine_get_smm, x86_machine_set_smm,
> > NULL, NULL);
> > @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
> > NULL, NULL);
> > object_class_property_set_description(oc, X86_MACHINE_ACPI,
> > "Enable ACPI");
> > +
> > + object_class_property_add_bool(oc, X86_MACHINE_HYPERV,
> > + x86_machine_get_hyperv, x86_machine_set_hyperv);
> > +
> > + object_class_property_set_description(oc, X86_MACHINE_HYPERV,
> > + "Enable Hyper-V enlightenments");
> > }
> >
> > static const TypeInfo x86_machine_info = {
> > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> > index 739fac50871b..598abd1be806 100644
> > --- a/include/hw/i386/x86.h
> > +++ b/include/hw/i386/x86.h
> > @@ -38,6 +38,9 @@ struct X86MachineClass {
> > bool save_tsc_khz;
> > /* Enables contiguous-apic-ID mode */
> > bool compat_apic_id_mode;
> > +
> > + /* Hyper-V features enabled with 'hyperv=on' */
> > + uint64_t default_hyperv_features;
> > };
> >
> > struct X86MachineState {
> > @@ -71,10 +74,14 @@ struct X86MachineState {
> > * will be translated to MSI messages in the address space.
> > */
> > AddressSpace *ioapic_as;
> > +
> > + /* Hyper-V emulation */
> > + bool hyperv_enabled;
> > };
> >
> > #define X86_MACHINE_SMM "smm"
> > #define X86_MACHINE_ACPI "acpi"
> > +#define X86_MACHINE_HYPERV "hyperv"
> >
> > #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86")
> > OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 83aca942d87c..63a931679d73 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -53,6 +53,7 @@
> > #include "sysemu/tcg.h"
> > #include "hw/qdev-properties.h"
> > #include "hw/i386/topology.h"
> > +#include "hw/i386/x86.h"
> > #ifndef CONFIG_USER_ONLY
> > #include "exec/address-spaces.h"
> > #include "hw/i386/apic_internal.h"
> > @@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> >
> > static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > {
> > + X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> > + X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> > + uint64_t feat;
> > size_t len;
> >
> > + if (x86ms->hyperv_enabled) {
> > + feat = x86mc->default_hyperv_features;
> > + /* Enlightened VMCS is only available on Intel/VMX */
> > + if (!cpu_has_vmx(&cpu->env)) {
> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > + }
> > +
> > + cpu->hyperv_features |= feat;
that will ignore features user explicitly doesn't want,
ex:
-machine hyperv=on -cpu foo,hv-foo=off
not sure we would like to introduce such invariant,
in normal qom property handling the latest set property should have effect
(all other invariants we have in x86 cpu property semantics are comming from legacy handling
and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
any other QOM object when it come to property handling)
anyways it's confusing a bit to have cpu flags to come from 2 different places
-cpu hyperv-use-preset=on,hv-foo=off
looks less confusing and will heave expected effect
> > + }
>
> I had to dequeue this because it doesn't compile with
> CONFIG_USER_ONLY:
>
> https://gitlab.com/ehabkost/qemu/-/jobs/916651017
>
> The easiest solution would be to wrap the new code in #ifndef
> CONFIG_USER_ONLY, but maybe we should try to move all
> X86Machine-specific code from cpu.c to
> hw/i386/x86.c:x86_cpu_pre_plug().
this looks to me like a preset of feature flags that belongs to CPU,
and machine code here only as a way to version subset of CPU features.
Is there a way to implement it without modifying machine?
for example versioned CPUs or maybe something like this:
for CLI:
-cpu hyperv-use-preset=on,hv-foo=off
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8d1a90c6cf..8828dcde8e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@ GlobalProperty hw_compat_5_0[] = {
{ "vmport", "x-signal-unsupported-cmd", "off" },
{ "vmport", "x-report-vmx-type", "off" },
{ "vmport", "x-cmds-v2", "off" },
+ { "cpu-foo", "hv-preset", "0xXXXX" }, // use compat props to keep old defaults
+ // it will be set before we return from object_new(cpu_type)
};
const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
diff --git a/slirp b/slirp
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
+Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 588f32e136..f0b511ce27 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7190,6 +7190,8 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
HYPERV_SPINLOCK_NEVER_RETRY),
+ DEFINE_PROP_UNIT64("hyperv-preset", X86CPU, hyperv_features_def, 0xYYYYY),
+ // prop_info should define custom setter/getter that will copy hyperv_features_def into hyperv_features
+ // moment "hyperv-use-preset=on" is processed, it will overwrite any previously set
+ // hv-foo but that's fine because user asked for it explictly
+ DEFINE_PROP("hyperv-use-preset", X86CPU, hyperv_use_preset, prop_info, bool),
DEFINE_PROP_BIT64("hv-relaxed", X86CPU, hyperv_features,
HYPERV_FEAT_RELAXED, 0),
DEFINE_PROP_BIT64("hv-vapic", X86CPU, hyperv_features,
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-12-18 17:13 ` Igor Mammedov
@ 2020-12-18 18:07 ` Eduardo Habkost
2020-12-21 13:24 ` Igor Mammedov
2021-01-04 12:54 ` Vitaly Kuznetsov
1 sibling, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2020-12-18 18:07 UTC (permalink / raw)
To: Igor Mammedov
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Fri, Dec 18, 2020 at 06:13:40PM +0100, Igor Mammedov wrote:
> On Wed, 16 Dec 2020 15:52:02 -0500
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
> > > Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
> > > requires listing all currently supported enlightenments ("hv_*" CPU
> > > features) explicitly. We do have a 'hv_passthrough' mode enabling
> > > everything but it can't be used in production as it prevents migration.
> > >
> > > Introduce a simple 'hyperv=on' option for all x86 machine types enabling
> > > all currently supported Hyper-V enlightenments. Later, when new
> > > enlightenments get implemented, we will be adding them to newer machine
> > > types only (by disabling them for legacy machine types) thus preserving
> > > migration.
> > >
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > [...]
> > > ---
> > > docs/hyperv.txt | 8 ++++++++
> > > hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++
> > > include/hw/i386/x86.h | 7 +++++++
> > > target/i386/cpu.c | 14 ++++++++++++++
> > > 4 files changed, 59 insertions(+)
> > >
> > > diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > > index 5df00da54fc4..1a76a07f8417 100644
> > > --- a/docs/hyperv.txt
> > > +++ b/docs/hyperv.txt
> > > @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor
> > > identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification
> > > and features are kept in leaves 0x40000100..0x40000101.
> > >
> > > +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
> > > +x86 machine type:
> > > +
> > > + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ...
> > > +
> > > +Note, new enlightenments are only added to the latest (in-develompent) machine
> > > +type, older machine types keep the list of the supported features intact to
> > > +safeguard migration.
> > >
> > > 3. Existing enlightenments
> > > ===========================
> > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > > index 5944fc44edca..57f27d56ecc6 100644
> > > --- a/hw/i386/x86.c
> > > +++ b/hw/i386/x86.c
> > > @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name,
> > > visit_type_OnOffAuto(v, name, &x86ms->acpi, errp);
> > > }
> > >
> > > +static bool x86_machine_get_hyperv(Object *obj, Error **errp)
> > > +{
> > > + X86MachineState *x86ms = X86_MACHINE(obj);
> > > +
> > > + return x86ms->hyperv_enabled;
> > > +}
> > > +
> > > +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
> > > +{
> > > + X86MachineState *x86ms = X86_MACHINE(obj);
> > > +
> > > + x86ms->hyperv_enabled = value;
> > > +}
> > > +
> > > static void x86_machine_initfn(Object *obj)
> > > {
> > > X86MachineState *x86ms = X86_MACHINE(obj);
> > > @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
> > > x86mc->save_tsc_khz = true;
> > > nc->nmi_monitor_handler = x86_nmi;
> > >
> > > + /* Hyper-V features enabled with 'hyperv=on' */
> > > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> I'd argue that feature bits do not belong to machine code at all.
> If we have to involve machine at all then it should be a set property/value pairs
> that machine will set on CPU object (I'm not convinced that doing it
> from machine code is good idea though).
The set of default hyperv features needs be defined by the
machine type somehow, we can't avoid that.
You are correct that the policy could be implemented using
compat_props, but I don't think we should block a patch just
because we're not using a pure QOM property-based interface to
implement that.
We need the external interface to be good, though:
>
[...]
> > > static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > > {
> > > + X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> > > + X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> > > + uint64_t feat;
> > > size_t len;
> > >
> > > + if (x86ms->hyperv_enabled) {
> > > + feat = x86mc->default_hyperv_features;
> > > + /* Enlightened VMCS is only available on Intel/VMX */
> > > + if (!cpu_has_vmx(&cpu->env)) {
> > > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > > + }
> > > +
> > > + cpu->hyperv_features |= feat;
> that will ignore features user explicitly doesn't want,
> ex:
> -machine hyperv=on -cpu foo,hv-foo=off
Oops, good point.
>
> not sure we would like to introduce such invariant,
> in normal qom property handling the latest set property should have effect
> (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> any other QOM object when it come to property handling)
>
> anyways it's confusing a bit to have cpu flags to come from 2 different places
>
> -cpu hyperv-use-preset=on,hv-foo=off
>
> looks less confusing and will heave expected effect
>
> > > + }
> >
> > I had to dequeue this because it doesn't compile with
> > CONFIG_USER_ONLY:
> >
> > https://gitlab.com/ehabkost/qemu/-/jobs/916651017
> >
> > The easiest solution would be to wrap the new code in #ifndef
> > CONFIG_USER_ONLY, but maybe we should try to move all
> > X86Machine-specific code from cpu.c to
> > hw/i386/x86.c:x86_cpu_pre_plug().
> this looks to me like a preset of feature flags that belongs to CPU,
> and machine code here only as a way to version subset of CPU features.
>
> Is there a way to implement it without modifying machine?
Maybe there is, but why modifying machine is a problem?
I agree the interface needs to be clear and consistent, though.
Maybe making it a -cpu option would make this clearer and more
consistent.
>
> for example versioned CPUs or maybe something like this:
>
> for CLI:
> -cpu hyperv-use-preset=on,hv-foo=off
In either case, we must clearly define what should happen if the
preset is (HYPERV_FEAT_X | HYPERV_FEAT_Y), and the command line
has:
-cpu foo,hv-A=on,hv-X=off,hyperv-use-preset=on,hv-B=on,hv-Y=off
or:
-machine hyperv=on -cpu foo,hv-A=on,hv-X=off,hv-B=on,hv-X=off
Personally, I don't care what the rules are, as long as: 1) they
are clearly defined and documented; 2) they support the use cases
we need to support.
An automated test case to make sure we don't break the rules
would be really welcome.
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8d1a90c6cf..8828dcde8e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -35,6 +35,7 @@ GlobalProperty hw_compat_5_0[] = {
> { "vmport", "x-signal-unsupported-cmd", "off" },
> { "vmport", "x-report-vmx-type", "off" },
> { "vmport", "x-cmds-v2", "off" },
> + { "cpu-foo", "hv-preset", "0xXXXX" }, // use compat props to keep old defaults
> + // it will be set before we return from object_new(cpu_type)
> };
> const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>
> diff --git a/slirp b/slirp
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
> +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 588f32e136..f0b511ce27 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7190,6 +7190,8 @@ static Property x86_cpu_properties[] = {
>
> DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
> HYPERV_SPINLOCK_NEVER_RETRY),
> + DEFINE_PROP_UNIT64("hyperv-preset", X86CPU, hyperv_features_def, 0xYYYYY),
> + // prop_info should define custom setter/getter that will copy hyperv_features_def into hyperv_features
> + // moment "hyperv-use-preset=on" is processed, it will overwrite any previously set
> + // hv-foo but that's fine because user asked for it explictly
> + DEFINE_PROP("hyperv-use-preset", X86CPU, hyperv_use_preset, prop_info, bool),
We don't need to use custom getters/setters with DEFINE_PROP, if
we can use object_class_property_add_bool().
I dislike custom getters/setters in either case, but maybe we
don't have a choice. Depending on the rules we agree upon above,
custom setters could become avoidable, or they could become a
necessity.
> DEFINE_PROP_BIT64("hv-relaxed", X86CPU, hyperv_features,
> HYPERV_FEAT_RELAXED, 0),
> DEFINE_PROP_BIT64("hv-vapic", X86CPU, hyperv_features,
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-12-18 18:07 ` Eduardo Habkost
@ 2020-12-21 13:24 ` Igor Mammedov
2020-12-21 19:47 ` Eduardo Habkost
0 siblings, 1 reply; 39+ messages in thread
From: Igor Mammedov @ 2020-12-21 13:24 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Fri, 18 Dec 2020 13:07:21 -0500
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Dec 18, 2020 at 06:13:40PM +0100, Igor Mammedov wrote:
> > On Wed, 16 Dec 2020 15:52:02 -0500
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
> > > > Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
> > > > requires listing all currently supported enlightenments ("hv_*" CPU
> > > > features) explicitly. We do have a 'hv_passthrough' mode enabling
> > > > everything but it can't be used in production as it prevents migration.
> > > >
> > > > Introduce a simple 'hyperv=on' option for all x86 machine types enabling
> > > > all currently supported Hyper-V enlightenments. Later, when new
> > > > enlightenments get implemented, we will be adding them to newer machine
> > > > types only (by disabling them for legacy machine types) thus preserving
> > > > migration.
> > > >
> > > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > [...]
> > > > ---
> > > > docs/hyperv.txt | 8 ++++++++
> > > > hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++
> > > > include/hw/i386/x86.h | 7 +++++++
> > > > target/i386/cpu.c | 14 ++++++++++++++
> > > > 4 files changed, 59 insertions(+)
> > > >
> > > > diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > > > index 5df00da54fc4..1a76a07f8417 100644
> > > > --- a/docs/hyperv.txt
> > > > +++ b/docs/hyperv.txt
> > > > @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor
> > > > identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification
> > > > and features are kept in leaves 0x40000100..0x40000101.
> > > >
> > > > +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
> > > > +x86 machine type:
> > > > +
> > > > + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ...
> > > > +
> > > > +Note, new enlightenments are only added to the latest (in-develompent) machine
> > > > +type, older machine types keep the list of the supported features intact to
> > > > +safeguard migration.
> > > >
> > > > 3. Existing enlightenments
> > > > ===========================
> > > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > > > index 5944fc44edca..57f27d56ecc6 100644
> > > > --- a/hw/i386/x86.c
> > > > +++ b/hw/i386/x86.c
> > > > @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name,
> > > > visit_type_OnOffAuto(v, name, &x86ms->acpi, errp);
> > > > }
> > > >
> > > > +static bool x86_machine_get_hyperv(Object *obj, Error **errp)
> > > > +{
> > > > + X86MachineState *x86ms = X86_MACHINE(obj);
> > > > +
> > > > + return x86ms->hyperv_enabled;
> > > > +}
> > > > +
> > > > +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
> > > > +{
> > > > + X86MachineState *x86ms = X86_MACHINE(obj);
> > > > +
> > > > + x86ms->hyperv_enabled = value;
> > > > +}
> > > > +
> > > > static void x86_machine_initfn(Object *obj)
> > > > {
> > > > X86MachineState *x86ms = X86_MACHINE(obj);
> > > > @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
> > > > x86mc->save_tsc_khz = true;
> > > > nc->nmi_monitor_handler = x86_nmi;
> > > >
> > > > + /* Hyper-V features enabled with 'hyperv=on' */
> > > > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > > > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > > > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > > > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > > > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > > > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > > > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > > > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > I'd argue that feature bits do not belong to machine code at all.
> > If we have to involve machine at all then it should be a set property/value pairs
> > that machine will set on CPU object (I'm not convinced that doing it
> > from machine code is good idea though).
>
> The set of default hyperv features needs be defined by the
> machine type somehow, we can't avoid that.
>
> You are correct that the policy could be implemented using
> compat_props, but I don't think we should block a patch just
> because we're not using a pure QOM property-based interface to
> implement that.
I'm fine with 1-4/5 patches but not with this one.
With this patch I don't agree with inventing
special semantics to property handling when it could
be done in a typical and consistent way (especially for
the sake of convenience).
> We need the external interface to be good, though:
>
> >
> [...]
> > > > static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > > > {
> > > > + X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> > > > + X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> > > > + uint64_t feat;
> > > > size_t len;
> > > >
> > > > + if (x86ms->hyperv_enabled) {
> > > > + feat = x86mc->default_hyperv_features;
> > > > + /* Enlightened VMCS is only available on Intel/VMX */
> > > > + if (!cpu_has_vmx(&cpu->env)) {
> > > > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > > > + }
> > > > +
> > > > + cpu->hyperv_features |= feat;
> > that will ignore features user explicitly doesn't want,
> > ex:
> > -machine hyperv=on -cpu foo,hv-foo=off
>
> Oops, good point.
>
>
> >
> > not sure we would like to introduce such invariant,
> > in normal qom property handling the latest set property should have effect
> > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > any other QOM object when it come to property handling)
> >
> > anyways it's confusing a bit to have cpu flags to come from 2 different places
> >
> > -cpu hyperv-use-preset=on,hv-foo=off
> >
> > looks less confusing and will heave expected effect
> >
> > > > + }
> > >
> > > I had to dequeue this because it doesn't compile with
> > > CONFIG_USER_ONLY:
> > >
> > > https://gitlab.com/ehabkost/qemu/-/jobs/916651017
> > >
> > > The easiest solution would be to wrap the new code in #ifndef
> > > CONFIG_USER_ONLY, but maybe we should try to move all
> > > X86Machine-specific code from cpu.c to
> > > hw/i386/x86.c:x86_cpu_pre_plug().
> > this looks to me like a preset of feature flags that belongs to CPU,
> > and machine code here only as a way to version subset of CPU features.
> >
> > Is there a way to implement it without modifying machine?
>
> Maybe there is, but why modifying machine is a problem?
1. it doesn't let do the job properly (realize time is too late)
2. unnecessarily pushes CPU specific logic to machine code,
it just doesn't belong there.
Sure we can do that here, then some where else and in the end
code becomes unmanageable mess.
> I agree the interface needs to be clear and consistent, though.
> Maybe making it a -cpu option would make this clearer and more
> consistent.
>
> >
> > for example versioned CPUs or maybe something like this:
> >
> > for CLI:
> > -cpu hyperv-use-preset=on,hv-foo=off
>
> In either case, we must clearly define what should happen if the
> preset is (HYPERV_FEAT_X | HYPERV_FEAT_Y), and the command line
> has:
>
> -cpu foo,hv-A=on,hv-X=off,hyperv-use-preset=on,hv-B=on,hv-Y=off
current x86 cpu code (it doesn't have typical properties handling
for keeping legacy semantics), it will basically reorder all features
with 'off' value to the end, so hv-X=off will still have an effect.
However I plan to deprecate those reordering semantics (x86/sparc cpus),
to make it consistent with typical property handling
(last set value overwrites any previously set one).
That will let us drop custom parsing of -cpu (quite a bit of code) and
more importantly make it consistent with -device/device_add cpu-foo.
> or:
>
> -machine hyperv=on -cpu foo,hv-A=on,hv-X=off,hv-B=on,hv-X=off
>
> Personally, I don't care what the rules are, as long as: 1) they
> are clearly defined and documented; 2) they support the use cases
> we need to support.
I'd like to stick with typical property handling rules, and resort to
inventing/using other invariant only if there is no other choice.
> An automated test case to make sure we don't break the rules
> would be really welcome.
>
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 8d1a90c6cf..8828dcde8e 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -35,6 +35,7 @@ GlobalProperty hw_compat_5_0[] = {
> > { "vmport", "x-signal-unsupported-cmd", "off" },
> > { "vmport", "x-report-vmx-type", "off" },
> > { "vmport", "x-cmds-v2", "off" },
> > + { "cpu-foo", "hv-preset", "0xXXXX" }, // use compat props to keep old defaults
> > + // it will be set before we return from object_new(cpu_type)
> > };
> > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> >
> > diff --git a/slirp b/slirp
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
> > +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 588f32e136..f0b511ce27 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -7190,6 +7190,8 @@ static Property x86_cpu_properties[] = {
> >
> > DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
> > HYPERV_SPINLOCK_NEVER_RETRY),
> > + DEFINE_PROP_UNIT64("hyperv-preset", X86CPU, hyperv_features_def, 0xYYYYY),
> > + // prop_info should define custom setter/getter that will copy hyperv_features_def into hyperv_features
> > + // moment "hyperv-use-preset=on" is processed, it will overwrite any previously set
> > + // hv-foo but that's fine because user asked for it explictly
> > + DEFINE_PROP("hyperv-use-preset", X86CPU, hyperv_use_preset, prop_info, bool),
>
> We don't need to use custom getters/setters with DEFINE_PROP, if
> we can use object_class_property_add_bool().
of cause, I've used DEFINE_PROP just as a possible example.
> I dislike custom getters/setters in either case, but maybe we
> don't have a choice. Depending on the rules we agree upon above,
> custom setters could become avoidable, or they could become a
> necessity.
I do dislike them too, but sometimes custom setters are convenient
as they allow to check if value is valid and let us implement non
trivial handling (like in this case) at property setting time.
(doing overwites)
> > DEFINE_PROP_BIT64("hv-relaxed", X86CPU, hyperv_features,
> > HYPERV_FEAT_RELAXED, 0),
> > DEFINE_PROP_BIT64("hv-vapic", X86CPU, hyperv_features,
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-12-21 13:24 ` Igor Mammedov
@ 2020-12-21 19:47 ` Eduardo Habkost
2020-12-21 20:39 ` David Hildenbrand
0 siblings, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2020-12-21 19:47 UTC (permalink / raw)
To: Igor Mammedov
Cc: Thomas Huth, Cornelia Huck, David Hildenbrand, Marcelo Tosatti,
Richard Henderson, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x, Paolo Bonzini,
Vitaly Kuznetsov
+s390 maintainers, a question about feature groups below:
On Mon, Dec 21, 2020 at 02:24:18PM +0100, Igor Mammedov wrote:
> On Fri, 18 Dec 2020 13:07:21 -0500
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Fri, Dec 18, 2020 at 06:13:40PM +0100, Igor Mammedov wrote:
> > > On Wed, 16 Dec 2020 15:52:02 -0500
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
> > > > > Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
> > > > > requires listing all currently supported enlightenments ("hv_*" CPU
> > > > > features) explicitly. We do have a 'hv_passthrough' mode enabling
> > > > > everything but it can't be used in production as it prevents migration.
> > > > >
> > > > > Introduce a simple 'hyperv=on' option for all x86 machine types enabling
> > > > > all currently supported Hyper-V enlightenments. Later, when new
> > > > > enlightenments get implemented, we will be adding them to newer machine
> > > > > types only (by disabling them for legacy machine types) thus preserving
> > > > > migration.
> > > > >
> > > > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
[...]
> > > > > @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
> > > > > x86mc->save_tsc_khz = true;
> > > > > nc->nmi_monitor_handler = x86_nmi;
> > > > >
> > > > > + /* Hyper-V features enabled with 'hyperv=on' */
> > > > > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > > > > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > > > > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > > > > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > > > > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > > > > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > > > > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > > > > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > > I'd argue that feature bits do not belong to machine code at all.
> > > If we have to involve machine at all then it should be a set property/value pairs
> > > that machine will set on CPU object (I'm not convinced that doing it
> > > from machine code is good idea though).
> >
> > The set of default hyperv features needs be defined by the
> > machine type somehow, we can't avoid that.
> >
> > You are correct that the policy could be implemented using
> > compat_props, but I don't think we should block a patch just
> > because we're not using a pure QOM property-based interface to
> > implement that.
> I'm fine with 1-4/5 patches but not with this one.
> With this patch I don't agree with inventing
> special semantics to property handling when it could
> be done in a typical and consistent way (especially for
> the sake of convenience).
>
>
> > We need the external interface to be good, though:
> >
> > >
> > [...]
> > > > > static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > > > > {
> > > > > + X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> > > > > + X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> > > > > + uint64_t feat;
> > > > > size_t len;
> > > > >
> > > > > + if (x86ms->hyperv_enabled) {
> > > > > + feat = x86mc->default_hyperv_features;
> > > > > + /* Enlightened VMCS is only available on Intel/VMX */
> > > > > + if (!cpu_has_vmx(&cpu->env)) {
> > > > > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > > > > + }
> > > > > +
> > > > > + cpu->hyperv_features |= feat;
> > > that will ignore features user explicitly doesn't want,
> > > ex:
> > > -machine hyperv=on -cpu foo,hv-foo=off
> >
> > Oops, good point.
> >
> >
> > >
> > > not sure we would like to introduce such invariant,
> > > in normal qom property handling the latest set property should have effect
> > > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > > any other QOM object when it come to property handling)
> > >
> > > anyways it's confusing a bit to have cpu flags to come from 2 different places
> > >
> > > -cpu hyperv-use-preset=on,hv-foo=off
> > >
> > > looks less confusing and will heave expected effect
> > >
> > > > > + }
> > > >
> > > > I had to dequeue this because it doesn't compile with
> > > > CONFIG_USER_ONLY:
> > > >
> > > > https://gitlab.com/ehabkost/qemu/-/jobs/916651017
> > > >
> > > > The easiest solution would be to wrap the new code in #ifndef
> > > > CONFIG_USER_ONLY, but maybe we should try to move all
> > > > X86Machine-specific code from cpu.c to
> > > > hw/i386/x86.c:x86_cpu_pre_plug().
> > > this looks to me like a preset of feature flags that belongs to CPU,
> > > and machine code here only as a way to version subset of CPU features.
> > >
> > > Is there a way to implement it without modifying machine?
> >
> > Maybe there is, but why modifying machine is a problem?
>
> 1. it doesn't let do the job properly (realize time is too late)
> 2. unnecessarily pushes CPU specific logic to machine code,
> it just doesn't belong there.
> Sure we can do that here, then some where else and in the end
> code becomes unmanageable mess.
>
> > I agree the interface needs to be clear and consistent, though.
> > Maybe making it a -cpu option would make this clearer and more
> > consistent.
> >
> > >
> > > for example versioned CPUs or maybe something like this:
> > >
> > > for CLI:
> > > -cpu hyperv-use-preset=on,hv-foo=off
> >
> > In either case, we must clearly define what should happen if the
> > preset is (HYPERV_FEAT_X | HYPERV_FEAT_Y), and the command line
> > has:
> >
> > -cpu foo,hv-A=on,hv-X=off,hyperv-use-preset=on,hv-B=on,hv-Y=off
>
> current x86 cpu code (it doesn't have typical properties handling
> for keeping legacy semantics), it will basically reorder all features
> with 'off' value to the end, so hv-X=off will still have an effect.
>
> However I plan to deprecate those reordering semantics (x86/sparc cpus),
> to make it consistent with typical property handling
> (last set value overwrites any previously set one).
>
> That will let us drop custom parsing of -cpu (quite a bit of code) and
> more importantly make it consistent with -device/device_add cpu-foo.
Right.
>
>
> > or:
> >
> > -machine hyperv=on -cpu foo,hv-A=on,hv-X=off,hv-B=on,hv-X=off
> >
> > Personally, I don't care what the rules are, as long as: 1) they
> > are clearly defined and documented; 2) they support the use cases
> > we need to support.
>
> I'd like to stick with typical property handling rules, and resort to
> inventing/using other invariant only if there is no other choice.
What would be the typical handling rules, in this case? I don't
remember other cases in x86 where a single property affects
multiple feature flags.
We have something similar on s390x, though. So, a question to
s390x maintainers:
If "G" is a feature group including the features X, Y, Z, what is
the result of:
-cpu foo,X=off,G=on,Y=off
Would X be enabled? Would Y be enabled?
I would expect X to be enabled and Y to be disabled, but I'd like
to confirm.
>
>
> > An automated test case to make sure we don't break the rules
> > would be really welcome.
> >
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 8d1a90c6cf..8828dcde8e 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -35,6 +35,7 @@ GlobalProperty hw_compat_5_0[] = {
> > > { "vmport", "x-signal-unsupported-cmd", "off" },
> > > { "vmport", "x-report-vmx-type", "off" },
> > > { "vmport", "x-cmds-v2", "off" },
> > > + { "cpu-foo", "hv-preset", "0xXXXX" }, // use compat props to keep old defaults
> > > + // it will be set before we return from object_new(cpu_type)
> > > };
> > > const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > >
> > > diff --git a/slirp b/slirp
> > > --- a/slirp
> > > +++ b/slirp
> > > @@ -1 +1 @@
> > > -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
> > > +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 588f32e136..f0b511ce27 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -7190,6 +7190,8 @@ static Property x86_cpu_properties[] = {
> > >
> > > DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
> > > HYPERV_SPINLOCK_NEVER_RETRY),
> > > + DEFINE_PROP_UNIT64("hyperv-preset", X86CPU, hyperv_features_def, 0xYYYYY),
> > > + // prop_info should define custom setter/getter that will copy hyperv_features_def into hyperv_features
> > > + // moment "hyperv-use-preset=on" is processed, it will overwrite any previously set
> > > + // hv-foo but that's fine because user asked for it explictly
> > > + DEFINE_PROP("hyperv-use-preset", X86CPU, hyperv_use_preset, prop_info, bool),
> >
> > We don't need to use custom getters/setters with DEFINE_PROP, if
> > we can use object_class_property_add_bool().
> of cause, I've used DEFINE_PROP just as a possible example.
>
> > I dislike custom getters/setters in either case, but maybe we
> > don't have a choice. Depending on the rules we agree upon above,
> > custom setters could become avoidable, or they could become a
> > necessity.
>
> I do dislike them too, but sometimes custom setters are convenient
> as they allow to check if value is valid and let us implement non
> trivial handling (like in this case) at property setting time.
> (doing overwites)
>
> > > DEFINE_PROP_BIT64("hv-relaxed", X86CPU, hyperv_features,
> > > HYPERV_FEAT_RELAXED, 0),
> > > DEFINE_PROP_BIT64("hv-vapic", X86CPU, hyperv_features,
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-12-21 19:47 ` Eduardo Habkost
@ 2020-12-21 20:39 ` David Hildenbrand
0 siblings, 0 replies; 39+ messages in thread
From: David Hildenbrand @ 2020-12-21 20:39 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Thomas Huth, Cornelia Huck, David Hildenbrand, Marcelo Tosatti,
Richard Henderson, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x, Paolo Bonzini, Igor Mammedov,
Vitaly Kuznetsov
> Am 21.12.2020 um 20:47 schrieb Eduardo Habkost <ehabkost@redhat.com>:
>
> +s390 maintainers, a question about feature groups below:
>
>> On Mon, Dec 21, 2020 at 02:24:18PM +0100, Igor Mammedov wrote:
>> On Fri, 18 Dec 2020 13:07:21 -0500
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Fri, Dec 18, 2020 at 06:13:40PM +0100, Igor Mammedov wrote:
>>>> On Wed, 16 Dec 2020 15:52:02 -0500
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>
>>>>> On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
>>>>>> Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
>>>>>> requires listing all currently supported enlightenments ("hv_*" CPU
>>>>>> features) explicitly. We do have a 'hv_passthrough' mode enabling
>>>>>> everything but it can't be used in production as it prevents migration.
>>>>>>
>>>>>> Introduce a simple 'hyperv=on' option for all x86 machine types enabling
>>>>>> all currently supported Hyper-V enlightenments. Later, when new
>>>>>> enlightenments get implemented, we will be adding them to newer machine
>>>>>> types only (by disabling them for legacy machine types) thus preserving
>>>>>> migration.
>>>>>>
>>>>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> [...]
>>>>>> @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
>>>>>> x86mc->save_tsc_khz = true;
>>>>>> nc->nmi_monitor_handler = x86_nmi;
>>>>>>
>>>>>> + /* Hyper-V features enabled with 'hyperv=on' */
>>>>>> + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
>>>>>> + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
>>>>>> + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
>>>>>> + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
>>>>>> + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
>>>>>> + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
>>>>>> + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
>>>>>> + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
>>>> I'd argue that feature bits do not belong to machine code at all.
>>>> If we have to involve machine at all then it should be a set property/value pairs
>>>> that machine will set on CPU object (I'm not convinced that doing it
>>>> from machine code is good idea though).
>>>
>>> The set of default hyperv features needs be defined by the
>>> machine type somehow, we can't avoid that.
>>>
>>> You are correct that the policy could be implemented using
>>> compat_props, but I don't think we should block a patch just
>>> because we're not using a pure QOM property-based interface to
>>> implement that.
>> I'm fine with 1-4/5 patches but not with this one.
>> With this patch I don't agree with inventing
>> special semantics to property handling when it could
>> be done in a typical and consistent way (especially for
>> the sake of convenience).
>>
>>
>>> We need the external interface to be good, though:
>>>
>>>>
>>> [...]
>>>>>> static void x86_cpu_hyperv_realize(X86CPU *cpu)
>>>>>> {
>>>>>> + X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
>>>>>> + X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
>>>>>> + uint64_t feat;
>>>>>> size_t len;
>>>>>>
>>>>>> + if (x86ms->hyperv_enabled) {
>>>>>> + feat = x86mc->default_hyperv_features;
>>>>>> + /* Enlightened VMCS is only available on Intel/VMX */
>>>>>> + if (!cpu_has_vmx(&cpu->env)) {
>>>>>> + feat &= ~BIT(HYPERV_FEAT_EVMCS);
>>>>>> + }
>>>>>> +
>>>>>> + cpu->hyperv_features |= feat;
>>>> that will ignore features user explicitly doesn't want,
>>>> ex:
>>>> -machine hyperv=on -cpu foo,hv-foo=off
>>>
>>> Oops, good point.
>>>
>>>
>>>>
>>>> not sure we would like to introduce such invariant,
>>>> in normal qom property handling the latest set property should have effect
>>>> (all other invariants we have in x86 cpu property semantics are comming from legacy handling
>>>> and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
>>>> any other QOM object when it come to property handling)
>>>>
>>>> anyways it's confusing a bit to have cpu flags to come from 2 different places
>>>>
>>>> -cpu hyperv-use-preset=on,hv-foo=off
>>>>
>>>> looks less confusing and will heave expected effect
>>>>
>>>>>> + }
>>>>>
>>>>> I had to dequeue this because it doesn't compile with
>>>>> CONFIG_USER_ONLY:
>>>>>
>>>>> https://gitlab.com/ehabkost/qemu/-/jobs/916651017
>>>>>
>>>>> The easiest solution would be to wrap the new code in #ifndef
>>>>> CONFIG_USER_ONLY, but maybe we should try to move all
>>>>> X86Machine-specific code from cpu.c to
>>>>> hw/i386/x86.c:x86_cpu_pre_plug().
>>>> this looks to me like a preset of feature flags that belongs to CPU,
>>>> and machine code here only as a way to version subset of CPU features.
>>>>
>>>> Is there a way to implement it without modifying machine?
>>>
>>> Maybe there is, but why modifying machine is a problem?
>>
>> 1. it doesn't let do the job properly (realize time is too late)
>> 2. unnecessarily pushes CPU specific logic to machine code,
>> it just doesn't belong there.
>> Sure we can do that here, then some where else and in the end
>> code becomes unmanageable mess.
>>
>>> I agree the interface needs to be clear and consistent, though.
>>> Maybe making it a -cpu option would make this clearer and more
>>> consistent.
>>>
>>>>
>>>> for example versioned CPUs or maybe something like this:
>>>>
>>>> for CLI:
>>>> -cpu hyperv-use-preset=on,hv-foo=off
>>>
>>> In either case, we must clearly define what should happen if the
>>> preset is (HYPERV_FEAT_X | HYPERV_FEAT_Y), and the command line
>>> has:
>>>
>>> -cpu foo,hv-A=on,hv-X=off,hyperv-use-preset=on,hv-B=on,hv-Y=off
>>
>> current x86 cpu code (it doesn't have typical properties handling
>> for keeping legacy semantics), it will basically reorder all features
>> with 'off' value to the end, so hv-X=off will still have an effect.
>>
>> However I plan to deprecate those reordering semantics (x86/sparc cpus),
>> to make it consistent with typical property handling
>> (last set value overwrites any previously set one).
>>
>> That will let us drop custom parsing of -cpu (quite a bit of code) and
>> more importantly make it consistent with -device/device_add cpu-foo.
>
> Right.
>
>>
>>
>>> or:
>>>
>>> -machine hyperv=on -cpu foo,hv-A=on,hv-X=off,hv-B=on,hv-X=off
>>>
>>> Personally, I don't care what the rules are, as long as: 1) they
>>> are clearly defined and documented; 2) they support the use cases
>>> we need to support.
>>
>> I'd like to stick with typical property handling rules, and resort to
>> inventing/using other invariant only if there is no other choice.
>
> What would be the typical handling rules, in this case? I don't
> remember other cases in x86 where a single property affects
> multiple feature flags.
>
> We have something similar on s390x, though. So, a question to
> s390x maintainers:
>
> If "G" is a feature group including the features X, Y, Z, what is
> the result of:
>
> -cpu foo,X=off,G=on,Y=off
>
> Would X be enabled? Would Y be enabled?
>
> I would expect X to be enabled and Y to be disabled, but I'd like
> to confirm.
IIRC, the last one wins. Properties are applied left to right.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2020-12-18 17:13 ` Igor Mammedov
2020-12-18 18:07 ` Eduardo Habkost
@ 2021-01-04 12:54 ` Vitaly Kuznetsov
2021-01-04 18:29 ` Eduardo Habkost
2021-01-04 23:04 ` Igor Mammedov
1 sibling, 2 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-01-04 12:54 UTC (permalink / raw)
To: Igor Mammedov, Eduardo Habkost; +Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel
Igor Mammedov <imammedo@redhat.com> writes:
>> >
>> > + /* Hyper-V features enabled with 'hyperv=on' */
>> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
>> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
>> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
>> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
>> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
>> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
>> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
>> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> I'd argue that feature bits do not belong to machine code at all.
> If we have to involve machine at all then it should be a set property/value pairs
> that machine will set on CPU object (I'm not convinced that doing it
> from machine code is good idea though).
>
These are 'features' and not feature bits. 'Bits' here are just our
internal (to QEMU) representation of which features are enable and which
are not, we could've just used booleans instead. These feature, when
enabled, will result in some CPUID changes (not 1:1) but I don't see how
it's different from
" -machine q35,accel=kvm "
which also results in CPUID changes.
The main reason for putting this to x86 machine type is versioning, as
we go along we will (hopefully) be implementing more and more Hyper-V
features but we want to provide 'one knob to rule them all' but do it in
a way that will allow migration. We already have 'hv_passthrough' for
CPU.
>> >
>> > + if (x86ms->hyperv_enabled) {
>> > + feat = x86mc->default_hyperv_features;
>> > + /* Enlightened VMCS is only available on Intel/VMX */
>> > + if (!cpu_has_vmx(&cpu->env)) {
>> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
>> > + }
>> > +
>> > + cpu->hyperv_features |= feat;
> that will ignore features user explicitly doesn't want,
> ex:
> -machine hyperv=on -cpu foo,hv-foo=off
>
Existing 'hv_passthrough' mode can also affect the result. Personally, I
don't see where 'hv-foo=off' is needed outside of debugging and these
use-cases can probably be covered by explicitly listing required
features but I'm not against making this work, shouldn't be hard.
> not sure we would like to introduce such invariant,
> in normal qom property handling the latest set property should have effect
> (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> any other QOM object when it come to property handling)
>
> anyways it's confusing a bit to have cpu flags to come from 2 different places
>
> -cpu hyperv-use-preset=on,hv-foo=off
>
> looks less confusing and will heave expected effect
>
Honestly, 'hyperv-use-preset' is confusing even to me :-)
What if we for a second stop thinking about Hyper-V features being CPU
features only, e.g. if we want to create Dynamic Memory or PTP or any
other Hyper-V specific device in a simple way? We'll have to put these
under machine type.
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-04 12:54 ` Vitaly Kuznetsov
@ 2021-01-04 18:29 ` Eduardo Habkost
2021-01-04 23:36 ` Igor Mammedov
2021-01-04 23:04 ` Igor Mammedov
1 sibling, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2021-01-04 18:29 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Igor Mammedov, Marcelo Tosatti, qemu-devel, Paolo Bonzini
On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> >> >
> >> > + /* Hyper-V features enabled with 'hyperv=on' */
> >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > I'd argue that feature bits do not belong to machine code at all.
> > If we have to involve machine at all then it should be a set property/value pairs
> > that machine will set on CPU object (I'm not convinced that doing it
> > from machine code is good idea though).
> >
>
> These are 'features' and not feature bits. 'Bits' here are just our
> internal (to QEMU) representation of which features are enable and which
> are not, we could've just used booleans instead. These feature, when
> enabled, will result in some CPUID changes (not 1:1) but I don't see how
> it's different from
>
> " -machine q35,accel=kvm "
>
> which also results in CPUID changes.
This is a good point, although having accel affect CPUID bits was
also a source of complexity for query-cpu-model-expansion and
other QMP queries.
>
> The main reason for putting this to x86 machine type is versioning, as
> we go along we will (hopefully) be implementing more and more Hyper-V
> features but we want to provide 'one knob to rule them all' but do it in
> a way that will allow migration. We already have 'hv_passthrough' for
> CPU.
I agree completely that the set of bits needs to be on
MachineClass. We just need to agree on the external interface.
>
> >> >
> >> > + if (x86ms->hyperv_enabled) {
> >> > + feat = x86mc->default_hyperv_features;
> >> > + /* Enlightened VMCS is only available on Intel/VMX */
> >> > + if (!cpu_has_vmx(&cpu->env)) {
> >> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> >> > + }
> >> > +
> >> > + cpu->hyperv_features |= feat;
> > that will ignore features user explicitly doesn't want,
> > ex:
> > -machine hyperv=on -cpu foo,hv-foo=off
> >
>
> Existing 'hv_passthrough' mode can also affect the result. Personally, I
> don't see where 'hv-foo=off' is needed outside of debugging and these
> use-cases can probably be covered by explicitly listing required
> features but I'm not against making this work, shouldn't be hard.
I'm all for not wasting time supporting use cases that are not
necessary in practice. We just need to document the expected
behavior clearly, whatever we decide to do.
>
> > not sure we would like to introduce such invariant,
> > in normal qom property handling the latest set property should have effect
> > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > any other QOM object when it come to property handling)
> >
> > anyways it's confusing a bit to have cpu flags to come from 2 different places
> >
> > -cpu hyperv-use-preset=on,hv-foo=off
> >
> > looks less confusing and will heave expected effect
> >
>
> Honestly, 'hyperv-use-preset' is confusing even to me :-)
>
> What if we for a second stop thinking about Hyper-V features being CPU
> features only, e.g. if we want to create Dynamic Memory or PTP or any
> other Hyper-V specific device in a simple way? We'll have to put these
> under machine type.
I agree. Hyper-V is not just a set of CPU features.
Also, those two approaches are not mutually exclusive.
"-machine hyperv=on" can be implemented internally using
"hyperv-use-preset=on" if necessary. I don't think it has to,
however.
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-04 12:54 ` Vitaly Kuznetsov
2021-01-04 18:29 ` Eduardo Habkost
@ 2021-01-04 23:04 ` Igor Mammedov
2021-01-05 11:50 ` Vitaly Kuznetsov
1 sibling, 1 reply; 39+ messages in thread
From: Igor Mammedov @ 2021-01-04 23:04 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
On Mon, 04 Jan 2021 13:54:32 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> >> >
> >> > + /* Hyper-V features enabled with 'hyperv=on' */
> >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > I'd argue that feature bits do not belong to machine code at all.
> > If we have to involve machine at all then it should be a set property/value pairs
> > that machine will set on CPU object (I'm not convinced that doing it
> > from machine code is good idea though).
> >
>
> These are 'features' and not feature bits. 'Bits' here are just our
> internal (to QEMU) representation of which features are enable and which
> are not, we could've just used booleans instead. These feature, when
> enabled, will result in some CPUID changes (not 1:1) but I don't see how
> it's different from
>
> " -machine q35,accel=kvm "
>
> which also results in CPUID changes.
>
> The main reason for putting this to x86 machine type is versioning, as
> we go along we will (hopefully) be implementing more and more Hyper-V
> features but we want to provide 'one knob to rule them all' but do it in
> a way that will allow migration. We already have 'hv_passthrough' for
> CPU.
for versioning device models (cpu included), we typically set some default in
device's ininfn, and if later on we need to change it to another default
we use compat properties to keep old default to old machine types.
For example using it with CPU look at pc_compat_3_1
>
> >> >
> >> > + if (x86ms->hyperv_enabled) {
> >> > + feat = x86mc->default_hyperv_features;
> >> > + /* Enlightened VMCS is only available on Intel/VMX */
> >> > + if (!cpu_has_vmx(&cpu->env)) {
> >> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> >> > + }
> >> > +
> >> > + cpu->hyperv_features |= feat;
> > that will ignore features user explicitly doesn't want,
> > ex:
> > -machine hyperv=on -cpu foo,hv-foo=off
> >
>
> Existing 'hv_passthrough' mode can also affect the result. Personally, I
> don't see where 'hv-foo=off' is needed outside of debugging and these
> use-cases can probably be covered by explicitly listing required
> features but I'm not against making this work, shouldn't be hard.
there is a lot of way to implement something, in this case point is to
keep it consistent with the way we handle cpu features/properties.
And not to make up new semantics unless there is no other way.
> > not sure we would like to introduce such invariant,
> > in normal qom property handling the latest set property should have effect
> > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > any other QOM object when it come to property handling)
> >
> > anyways it's confusing a bit to have cpu flags to come from 2 different places
> >
> > -cpu hyperv-use-preset=on,hv-foo=off
> >
> > looks less confusing and will heave expected effect
> >
>
> Honestly, 'hyperv-use-preset' is confusing even to me :-)
that was just an example
>
> What if we for a second stop thinking about Hyper-V features being CPU
> features only, e.g. if we want to create Dynamic Memory or PTP or any
> other Hyper-V specific device in a simple way? We'll have to put these
> under machine type.
ideally it would be a property of device that implements the feature
and machine might enable it depending on its own properties defaults,
but if you change the default behavior of the device model, you do
it in device model and use compat properties infrastructure to keep
old machine types happy.
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-04 18:29 ` Eduardo Habkost
@ 2021-01-04 23:36 ` Igor Mammedov
2021-01-05 14:34 ` Eduardo Habkost
0 siblings, 1 reply; 39+ messages in thread
From: Igor Mammedov @ 2021-01-04 23:36 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Mon, 4 Jan 2021 13:29:06 -0500
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov wrote:
> > Igor Mammedov <imammedo@redhat.com> writes:
> >
> > >> >
> > >> > + /* Hyper-V features enabled with 'hyperv=on' */
> > >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > > I'd argue that feature bits do not belong to machine code at all.
> > > If we have to involve machine at all then it should be a set property/value pairs
> > > that machine will set on CPU object (I'm not convinced that doing it
> > > from machine code is good idea though).
> > >
> >
> > These are 'features' and not feature bits. 'Bits' here are just our
> > internal (to QEMU) representation of which features are enable and which
> > are not, we could've just used booleans instead. These feature, when
> > enabled, will result in some CPUID changes (not 1:1) but I don't see how
> > it's different from
> >
> > " -machine q35,accel=kvm "
> >
> > which also results in CPUID changes.
>
> This is a good point, although having accel affect CPUID bits was
> also a source of complexity for query-cpu-model-expansion and
> other QMP queries.
why was, it's still a headache (mutating CPU models depending on accelerator)
>
> >
> > The main reason for putting this to x86 machine type is versioning, as
> > we go along we will (hopefully) be implementing more and more Hyper-V
> > features but we want to provide 'one knob to rule them all' but do it in
> > a way that will allow migration. We already have 'hv_passthrough' for
> > CPU.
>
> I agree completely that the set of bits needs to be on
> MachineClass. We just need to agree on the external interface.
That's where I disagree,
let me exaggerate for demo purpose:
- let's move all CPU models feature defaults to MachineClass and forget about compat properties
since in that case we can opencode changes in machine_class_init
It's rather hard code integration between device models, which we try
to avoid and still refactoring QEMU code to get rid of it.
(sure it works until it's not and someone else need to rewrite half of QEMU
to accomplish it's own task because we mixed things together)
>
> >
> > >> >
> > >> > + if (x86ms->hyperv_enabled) {
> > >> > + feat = x86mc->default_hyperv_features;
> > >> > + /* Enlightened VMCS is only available on Intel/VMX */
> > >> > + if (!cpu_has_vmx(&cpu->env)) {
> > >> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > >> > + }
> > >> > +
> > >> > + cpu->hyperv_features |= feat;
> > > that will ignore features user explicitly doesn't want,
> > > ex:
> > > -machine hyperv=on -cpu foo,hv-foo=off
> > >
> >
> > Existing 'hv_passthrough' mode can also affect the result. Personally, I
> > don't see where 'hv-foo=off' is needed outside of debugging and these
> > use-cases can probably be covered by explicitly listing required
> > features but I'm not against making this work, shouldn't be hard.
>
> I'm all for not wasting time supporting use cases that are not
> necessary in practice. We just need to document the expected
> behavior clearly, whatever we decide to do.
documenting is good, but if it adds new semantics to how CPU features are handled
users up the stack will need code it up as well and juggle with
-machine + -cpu + -device cpu-foo
not to mention poor developers who will have to figure out why we do
set CPU properties in multiple different ways.
however if we add it as CPU properties that behave the same way as other
properties, all mgmt has to do is expose new property to user for usage.
it even more true when building machine from QMP interface would be available,
where we would want '-device foo' more or less the same way instead of
special casing some of them, i.e. I'd rather have one device to configure,
instead of doing it in multiple places. It's not possible in reality
but for new code we should try to minimize split brain issues.
> >
> > > not sure we would like to introduce such invariant,
> > > in normal qom property handling the latest set property should have effect
> > > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > > any other QOM object when it come to property handling)
> > >
> > > anyways it's confusing a bit to have cpu flags to come from 2 different places
> > >
> > > -cpu hyperv-use-preset=on,hv-foo=off
> > >
> > > looks less confusing and will heave expected effect
> > >
> >
> > Honestly, 'hyperv-use-preset' is confusing even to me :-)
> >
> > What if we for a second stop thinking about Hyper-V features being CPU
> > features only, e.g. if we want to create Dynamic Memory or PTP or any
> > other Hyper-V specific device in a simple way? We'll have to put these
> > under machine type.
>
> I agree. Hyper-V is not just a set of CPU features.
me too,
however in this case we are talking about a set of cpu features,
if there is no way to implement it as cpu properties + compat properties
and requires opencodding it within machine code it might be fine
but I fail to see a very good reason for doing that at this momment.
>
> Also, those two approaches are not mutually exclusive.
> "-machine hyperv=on" can be implemented internally using
> "hyperv-use-preset=on" if necessary. I don't think it has to,
> however.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-04 23:04 ` Igor Mammedov
@ 2021-01-05 11:50 ` Vitaly Kuznetsov
2021-01-05 16:03 ` Igor Mammedov
0 siblings, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-01-05 11:50 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
Igor Mammedov <imammedo@redhat.com> writes:
> On Mon, 04 Jan 2021 13:54:32 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Igor Mammedov <imammedo@redhat.com> writes:
>>
>> >> >
>> >> > + /* Hyper-V features enabled with 'hyperv=on' */
>> >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
>> >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
>> >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
>> >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
>> >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
>> >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
>> >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
>> >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
>> > I'd argue that feature bits do not belong to machine code at all.
>> > If we have to involve machine at all then it should be a set property/value pairs
>> > that machine will set on CPU object (I'm not convinced that doing it
>> > from machine code is good idea though).
>> >
>>
>> These are 'features' and not feature bits. 'Bits' here are just our
>> internal (to QEMU) representation of which features are enable and which
>> are not, we could've just used booleans instead. These feature, when
>> enabled, will result in some CPUID changes (not 1:1) but I don't see how
>> it's different from
>>
>> " -machine q35,accel=kvm "
>>
>> which also results in CPUID changes.
>>
>> The main reason for putting this to x86 machine type is versioning, as
>> we go along we will (hopefully) be implementing more and more Hyper-V
>> features but we want to provide 'one knob to rule them all' but do it in
>> a way that will allow migration. We already have 'hv_passthrough' for
>> CPU.
>
> for versioning device models (cpu included), we typically set some default in
> device's ininfn, and if later on we need to change it to another default
> we use compat properties to keep old default to old machine types.
>
> For example using it with CPU look at pc_compat_3_1
>
The tricky part for Hyper-V enlightenments is that we have to keep them
all off as the default when it wasn't explicitly requested as they're
only needed for Windows guests so one way or another we need a new knob
to enable the default-good-set.
>> What if we for a second stop thinking about Hyper-V features being CPU
>> features only, e.g. if we want to create Dynamic Memory or PTP or any
>> other Hyper-V specific device in a simple way? We'll have to put these
>> under machine type.
> ideally it would be a property of device that implements the feature
> and machine might enable it depending on its own properties defaults,
> but if you change the default behavior of the device model, you do
> it in device model and use compat properties infrastructure to keep
> old machine types happy.
This would work nicely if we were to enable some of the Hyper-V
enlightenments by default for new machine types and then turn them off
with compat properties. We are in a different situation though, we want
one knob which will tell us 'enable the default good set' and then we
need to subtract something from this set because e.g. our machine type
is old. In case the knob is, as you suggest, in CPU properties
('hv_default=on' or something like that) we'll have to play dirty games
in machine init funtion again: go to CPU device options and check if
'hv_default=on' was requested. If yes, then we enable all Hyper-V
enlightenments and do the subtraction according to machine version. And
what's even more weird, that we'll have to use 'hv_default=on' CPU flag
as an indication to create non-CPU devices. Much easier if the knob is a
property of machine type itself.
We can, of course, create a parallel (to the existing one) set of
Hyper-V properties which are going to be enabled by default (and
blacklisted by compat properties) and then later when CPU object is
created we'll set CPU properties according to these 'default' properties
but I hardly see a benefit in complicating stuff that much.
Also, compat properties are not the only thing we take into
consideration when creating an old machine type today. E.g.:
static void pc_q35_3_1_machine_options(MachineClass *m)
{
PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
pc_q35_4_0_machine_options(m);
m->default_kernel_irqchip_split = false;
pcmc->do_not_add_smb_acpi = true;
m->smbus_no_migration_support = true;
m->alias = NULL;
pcmc->pvh_enabled = false;
compat_props_add(m->compat_props, hw_compat_3_1, hw_compat_3_1_len);
compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
}
and that's exactly what I was thinking about for Hyper-V enlightenments:
when a new one is introduced we'll turn it off by default for new
machine types, no matter if it's going to be a CPU property or a new
device.
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-04 23:36 ` Igor Mammedov
@ 2021-01-05 14:34 ` Eduardo Habkost
2021-01-05 15:10 ` Vitaly Kuznetsov
2021-01-05 16:31 ` Igor Mammedov
0 siblings, 2 replies; 39+ messages in thread
From: Eduardo Habkost @ 2021-01-05 14:34 UTC (permalink / raw)
To: Igor Mammedov
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote:
> On Mon, 4 Jan 2021 13:29:06 -0500
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov wrote:
> > > Igor Mammedov <imammedo@redhat.com> writes:
> > >
> > > >> >
> > > >> > + /* Hyper-V features enabled with 'hyperv=on' */
> > > >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > > >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > > >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > > >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > > >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > > >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > > >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > > >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > > > I'd argue that feature bits do not belong to machine code at all.
> > > > If we have to involve machine at all then it should be a set property/value pairs
> > > > that machine will set on CPU object (I'm not convinced that doing it
> > > > from machine code is good idea though).
> > > >
> > >
> > > These are 'features' and not feature bits. 'Bits' here are just our
> > > internal (to QEMU) representation of which features are enable and which
> > > are not, we could've just used booleans instead. These feature, when
> > > enabled, will result in some CPUID changes (not 1:1) but I don't see how
> > > it's different from
> > >
> > > " -machine q35,accel=kvm "
> > >
> > > which also results in CPUID changes.
> >
> > This is a good point, although having accel affect CPUID bits was
> > also a source of complexity for query-cpu-model-expansion and
> > other QMP queries.
>
> why was, it's still a headache (mutating CPU models depending on accelerator)
>
> >
> > >
> > > The main reason for putting this to x86 machine type is versioning, as
> > > we go along we will (hopefully) be implementing more and more Hyper-V
> > > features but we want to provide 'one knob to rule them all' but do it in
> > > a way that will allow migration. We already have 'hv_passthrough' for
> > > CPU.
> >
> > I agree completely that the set of bits needs to be on
> > MachineClass. We just need to agree on the external interface.
> That's where I disagree,
> let me exaggerate for demo purpose:
> - let's move all CPU models feature defaults to MachineClass and forget about compat properties
> since in that case we can opencode changes in machine_class_init
I don't see your point here. compat_props is also part of
MachineClass.
>
> It's rather hard code integration between device models, which we try
> to avoid and still refactoring QEMU code to get rid of it.
> (sure it works until it's not and someone else need to rewrite half of QEMU
> to accomplish it's own task because we mixed things together)
I don't see why using a X86CPU-specific API that is not based on
QOM properties is hard code integration. compat_props is not the
only allowed API for machines to communicate with devices.
>
> >
> > >
> > > >> >
> > > >> > + if (x86ms->hyperv_enabled) {
> > > >> > + feat = x86mc->default_hyperv_features;
> > > >> > + /* Enlightened VMCS is only available on Intel/VMX */
> > > >> > + if (!cpu_has_vmx(&cpu->env)) {
> > > >> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > > >> > + }
> > > >> > +
> > > >> > + cpu->hyperv_features |= feat;
> > > > that will ignore features user explicitly doesn't want,
> > > > ex:
> > > > -machine hyperv=on -cpu foo,hv-foo=off
> > > >
> > >
> > > Existing 'hv_passthrough' mode can also affect the result. Personally, I
> > > don't see where 'hv-foo=off' is needed outside of debugging and these
> > > use-cases can probably be covered by explicitly listing required
> > > features but I'm not against making this work, shouldn't be hard.
> >
> > I'm all for not wasting time supporting use cases that are not
> > necessary in practice. We just need to document the expected
> > behavior clearly, whatever we decide to do.
>
> documenting is good, but if it adds new semantics to how CPU features are handled
> users up the stack will need code it up as well and juggle with
> -machine + -cpu + -device cpu-foo
> not to mention poor developers who will have to figure out why we do
> set CPU properties in multiple different ways.
>
> however if we add it as CPU properties that behave the same way as other
> properties, all mgmt has to do is expose new property to user for usage.
I think we need to be careful here. Sometimes just exposing the
QOM properties used to implemented a feature is not the best user
interface. e.g.: even if using compat_props for implementing the
hyperv features preset, that doesn't automatically mean we want
hyperv=on to be a -cpu option.
I would even argue we shouldn't be focusing on implementation
details (like we are doing right now) until the desired external
interface is described clearly.
>
> it even more true when building machine from QMP interface would be available,
> where we would want '-device foo' more or less the same way instead of
> special casing some of them, i.e. I'd rather have one device to configure,
> instead of doing it in multiple places. It's not possible in reality
> but for new code we should try to minimize split brain issues.
Is split brain a practical problem here? If the new behavior is
implemented in x86_cpu_realizefn() or x86_cpu_pre_plug(), we know
it's going to affect all CPU objects.
>
> > >
> > > > not sure we would like to introduce such invariant,
> > > > in normal qom property handling the latest set property should have effect
> > > > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > > > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > > > any other QOM object when it come to property handling)
> > > >
> > > > anyways it's confusing a bit to have cpu flags to come from 2 different places
> > > >
> > > > -cpu hyperv-use-preset=on,hv-foo=off
> > > >
> > > > looks less confusing and will heave expected effect
> > > >
> > >
> > > Honestly, 'hyperv-use-preset' is confusing even to me :-)
> > >
> > > What if we for a second stop thinking about Hyper-V features being CPU
> > > features only, e.g. if we want to create Dynamic Memory or PTP or any
> > > other Hyper-V specific device in a simple way? We'll have to put these
> > > under machine type.
> >
> > I agree. Hyper-V is not just a set of CPU features.
> me too,
> however in this case we are talking about a set of cpu features,
> if there is no way to implement it as cpu properties + compat properties
> and requires opencodding it within machine code it might be fine
> but I fail to see a very good reason for doing that at this momment.
The reason would be just simplicity of implementation.
I understand there are reasons to suggest using compat_props if
it makes things simpler, but I don't see why we would reject a
patch because the implementation is not based purely on
compat_props.
I will let Vitaly to decide how to proceed, based on our
feedback. I encourage him to use compat_props like you suggest,
but I don't plan to make this a requirement.
>
> >
> > Also, those two approaches are not mutually exclusive.
> > "-machine hyperv=on" can be implemented internally using
> > "hyperv-use-preset=on" if necessary. I don't think it has to,
> > however.
>
>
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 14:34 ` Eduardo Habkost
@ 2021-01-05 15:10 ` Vitaly Kuznetsov
2021-01-05 16:33 ` Igor Mammedov
2021-01-05 16:31 ` Igor Mammedov
1 sibling, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-01-05 15:10 UTC (permalink / raw)
To: Eduardo Habkost, Igor Mammedov; +Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote:
>>
>> documenting is good, but if it adds new semantics to how CPU features are handled
>> users up the stack will need code it up as well and juggle with
>> -machine + -cpu + -device cpu-foo
>> not to mention poor developers who will have to figure out why we do
>> set CPU properties in multiple different ways.
>>
>> however if we add it as CPU properties that behave the same way as other
>> properties, all mgmt has to do is expose new property to user for usage.
>
> I think we need to be careful here. Sometimes just exposing the
> QOM properties used to implemented a feature is not the best user
> interface. e.g.: even if using compat_props for implementing the
> hyperv features preset, that doesn't automatically mean we want
> hyperv=on to be a -cpu option.
>
> I would even argue we shouldn't be focusing on implementation
> details (like we are doing right now) until the desired external
> interface is described clearly.
I agree, the interface is definitely more important than the
implementation here. AFAIU we have two options suggested:
1) 'hyperv=on' option for x86 machine types.
Pros: we can use it later to create non-CPU Hyper-V devices
(e.g. Vmbus).
Cons: two different places for the currently existing Hyper-V features
enablement (-cpu and -machine), non-standard way of doing things
(code-wise).
2) 'hv_default=on' -cpu option
Pros: Single place to enable all Hyper-V enlightenments, we can make it
mutually exclusive with other hv_* options including hv_passthrough
(clear semantics).
Cons: This can't be reused to create non-CPU objects in the future and
so upper layers will (again) need to be modified.
There's probably more, please feel free to add.
>> however in this case we are talking about a set of cpu features,
>> if there is no way to implement it as cpu properties + compat properties
>> and requires opencodding it within machine code it might be fine
>> but I fail to see a very good reason for doing that at this momment.
>
> The reason would be just simplicity of implementation.
>
> I understand there are reasons to suggest using compat_props if
> it makes things simpler, but I don't see why we would reject a
> patch because the implementation is not based purely on
> compat_props.
>
> I will let Vitaly to decide how to proceed, based on our
> feedback. I encourage him to use compat_props like you suggest,
> but I don't plan to make this a requirement.
>
Like I replied to Igor in a parallel thread, I hardly see how using
compat_props can simplify things in case we decide to keep 'hyperv=on' a
machine type option. It doesn't seem to fit our use-case when we need a
mechanism to alter CPU properties for the current machine type as well
as subtract some features for the old ones. If we, however, decide that
'-cpu' option is better, then we can try to make it work (but the
implementation won't be straitforward either).
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 11:50 ` Vitaly Kuznetsov
@ 2021-01-05 16:03 ` Igor Mammedov
2021-01-05 16:31 ` Vitaly Kuznetsov
0 siblings, 1 reply; 39+ messages in thread
From: Igor Mammedov @ 2021-01-05 16:03 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
On Tue, 05 Jan 2021 12:50:05 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> > On Mon, 04 Jan 2021 13:54:32 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >>
> >> >> >
> >> >> > + /* Hyper-V features enabled with 'hyperv=on' */
> >> >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> >> >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> >> >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> >> >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> >> >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> >> >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> >> >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> >> >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> >> > I'd argue that feature bits do not belong to machine code at all.
> >> > If we have to involve machine at all then it should be a set property/value pairs
> >> > that machine will set on CPU object (I'm not convinced that doing it
> >> > from machine code is good idea though).
> >> >
> >>
> >> These are 'features' and not feature bits. 'Bits' here are just our
> >> internal (to QEMU) representation of which features are enable and which
> >> are not, we could've just used booleans instead. These feature, when
> >> enabled, will result in some CPUID changes (not 1:1) but I don't see how
> >> it's different from
> >>
> >> " -machine q35,accel=kvm "
> >>
> >> which also results in CPUID changes.
> >>
> >> The main reason for putting this to x86 machine type is versioning, as
> >> we go along we will (hopefully) be implementing more and more Hyper-V
> >> features but we want to provide 'one knob to rule them all' but do it in
> >> a way that will allow migration. We already have 'hv_passthrough' for
> >> CPU.
> >
> > for versioning device models (cpu included), we typically set some default in
> > device's ininfn, and if later on we need to change it to another default
> > we use compat properties to keep old default to old machine types.
> >
> > For example using it with CPU look at pc_compat_3_1
> >
>
> The tricky part for Hyper-V enlightenments is that we have to keep them
> all off as the default when it wasn't explicitly requested as they're
> only needed for Windows guests so one way or another we need a new knob
> to enable the default-good-set.
>
> >> What if we for a second stop thinking about Hyper-V features being CPU
> >> features only, e.g. if we want to create Dynamic Memory or PTP or any
> >> other Hyper-V specific device in a simple way? We'll have to put these
> >> under machine type.
> > ideally it would be a property of device that implements the feature
> > and machine might enable it depending on its own properties defaults,
> > but if you change the default behavior of the device model, you do
> > it in device model and use compat properties infrastructure to keep
> > old machine types happy.
>
> This would work nicely if we were to enable some of the Hyper-V
> enlightenments by default for new machine types and then turn them off
> with compat properties. We are in a different situation though, we want
> one knob which will tell us 'enable the default good set' and then we
> need to subtract something from this set because e.g. our machine type
> is old. In case the knob is, as you suggest, in CPU properties
> ('hv_default=on' or something like that) we'll have to play dirty games
> in machine init funtion again: go to CPU device options and check if
> 'hv_default=on' was requested. If yes, then we enable all Hyper-V
> enlightenments and do the subtraction according to machine version. And
> what's even more weird,
I think there is a misunderstanding, idea was:
cpu_initfn() {
//current set
cpu->default_hyperv_cpu_features = ACD
}
compat_props_5.1 {
cpu.default_hyperv_cpu_features = AB
}
compat_props_5.2 {
cpu.default_hyperv_cpu_features = ABC
}
and cpu property 'hv_default=on' should apply hv specific default set to
CPU when it's provided by user.
Simple as that, the scope of the patch was on CPU features not other devices,
and that keeps property semantics simple and clean
-cpu foo,hv_default=on,hv_foo1=on,hv_foo2=off
properties just any other are parsed and applied from left to right and no extra code
is necessary.
if we try to do the same at cpu.realize() time like this patch (cpu_pre_plug is the same)
based on '-machine hyperv=on', it won't work because realize() time is too late and
we end up loosing 'hv_foo2=off' and/or 'hv_foo1=on' which creates different semantics, and I wouldn't
write off 'hv_foo2=off' as useless, as it might unbreak some guests which don't like
'hv_foo2' for some reason.
to make '-machine hyperv=on' work nice with user set properties we either:
- at realize time, need to know if 'hv_foo2=off' was set by user and re-apply it on top of
hv_default=on (there is no API for that at the moment), and maybe we need to know order
in which properties are specified. which is messy and complex.
- potentially '-machine hyperv=on' may push 'cpu.hv_default=on' global property
like '-cpu foo,features' does. We need just need to make up mind on the order.
I'd go for
1. '-machine hyperv=on' => add_global('cpu.hv_default=on')
2. parse '-cpu' and/or -device cpu,features
In the end I'm not against '-machine hyperv=on', but it complicates things
compared to just
cpu.default_hyperv_cpu_features + cpu.hv_default + -cpu foo,hv_default=on when desired
+ using compat props for versioning.
And if -machine hyperv=on is 'must have' thing, it's fine as long as doesn't
create special cases for property parsing semantics.
> that we'll have to use 'hv_default=on' CPU flag
> as an indication to create non-CPU devices. Much easier if the knob is a
> property of machine type itself.
I was talking about CPU features/properties only, it doesn't apply to other devices.
It makes sense for machine to have a knob to create onboard hyperv specific
devices if there is any (do we have any?).
If there aren't any currently, I wouldn't bother with machine knob
and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
like any other cpu feature.
> We can, of course, create a parallel (to the existing one) set of
> Hyper-V properties which are going to be enabled by default (and
> blacklisted by compat properties) and then later when CPU object is
> created we'll set CPU properties according to these 'default' properties
> but I hardly see a benefit in complicating stuff that much.
> Also, compat properties are not the only thing we take into
> consideration when creating an old machine type today. E.g.:
>
> static void pc_q35_3_1_machine_options(MachineClass *m)
> {
> PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>
> pc_q35_4_0_machine_options(m);
> m->default_kernel_irqchip_split = false;
> pcmc->do_not_add_smb_acpi = true;
> m->smbus_no_migration_support = true;
> m->alias = NULL;
> pcmc->pvh_enabled = false;
> compat_props_add(m->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
> }
>
> and that's exactly what I was thinking about for Hyper-V enlightenments:
> when a new one is introduced we'll turn it off by default for new
> machine types, no matter if it's going to be a CPU property or a new
> device.
you lost me here, I'm not sure what are you talking about.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 14:34 ` Eduardo Habkost
2021-01-05 15:10 ` Vitaly Kuznetsov
@ 2021-01-05 16:31 ` Igor Mammedov
2021-01-05 17:02 ` Vitaly Kuznetsov
2021-01-05 18:19 ` Eduardo Habkost
1 sibling, 2 replies; 39+ messages in thread
From: Igor Mammedov @ 2021-01-05 16:31 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Tue, 5 Jan 2021 09:34:31 -0500
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote:
> > On Mon, 4 Jan 2021 13:29:06 -0500
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov wrote:
> > > > Igor Mammedov <imammedo@redhat.com> writes:
> > > >
> > > > >> >
> > > > >> > + /* Hyper-V features enabled with 'hyperv=on' */
> > > > >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > > > >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > > > >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > > > >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > > > >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > > > >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > > > >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > > > >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > > > > I'd argue that feature bits do not belong to machine code at all.
> > > > > If we have to involve machine at all then it should be a set property/value pairs
> > > > > that machine will set on CPU object (I'm not convinced that doing it
> > > > > from machine code is good idea though).
> > > > >
> > > >
> > > > These are 'features' and not feature bits. 'Bits' here are just our
> > > > internal (to QEMU) representation of which features are enable and which
> > > > are not, we could've just used booleans instead. These feature, when
> > > > enabled, will result in some CPUID changes (not 1:1) but I don't see how
> > > > it's different from
> > > >
> > > > " -machine q35,accel=kvm "
> > > >
> > > > which also results in CPUID changes.
> > >
> > > This is a good point, although having accel affect CPUID bits was
> > > also a source of complexity for query-cpu-model-expansion and
> > > other QMP queries.
> >
> > why was, it's still a headache (mutating CPU models depending on accelerator)
> >
> > >
> > > >
> > > > The main reason for putting this to x86 machine type is versioning, as
> > > > we go along we will (hopefully) be implementing more and more Hyper-V
> > > > features but we want to provide 'one knob to rule them all' but do it in
> > > > a way that will allow migration. We already have 'hv_passthrough' for
> > > > CPU.
> > >
> > > I agree completely that the set of bits needs to be on
> > > MachineClass. We just need to agree on the external interface.
> > That's where I disagree,
> > let me exaggerate for demo purpose:
> > - let's move all CPU models feature defaults to MachineClass and forget about compat properties
> > since in that case we can opencode changes in machine_class_init
>
> I don't see your point here. compat_props is also part of
> MachineClass.
they are but compat_props are data and we typically use them for
keeping old behavior for devices, all it needs is adding a line
to set old property value.
While class_init is typically used for altering machine specific
behavior, sure it can be used to patch up device but that's
a bit more ugly (need to add a field to MachineClass to key off
and the somehow wire it up to affected device).
> >
> > It's rather hard code integration between device models, which we try
> > to avoid and still refactoring QEMU code to get rid of it.
> > (sure it works until it's not and someone else need to rewrite half of QEMU
> > to accomplish it's own task because we mixed things together)
>
> I don't see why using a X86CPU-specific API that is not based on
> QOM properties is hard code integration. compat_props is not the
> only allowed API for machines to communicate with devices.
>
> >
> > >
> > > >
> > > > >> >
> > > > >> > + if (x86ms->hyperv_enabled) {
> > > > >> > + feat = x86mc->default_hyperv_features;
> > > > >> > + /* Enlightened VMCS is only available on Intel/VMX */
> > > > >> > + if (!cpu_has_vmx(&cpu->env)) {
> > > > >> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > > > >> > + }
> > > > >> > +
> > > > >> > + cpu->hyperv_features |= feat;
> > > > > that will ignore features user explicitly doesn't want,
> > > > > ex:
> > > > > -machine hyperv=on -cpu foo,hv-foo=off
> > > > >
> > > >
> > > > Existing 'hv_passthrough' mode can also affect the result. Personally, I
> > > > don't see where 'hv-foo=off' is needed outside of debugging and these
> > > > use-cases can probably be covered by explicitly listing required
> > > > features but I'm not against making this work, shouldn't be hard.
> > >
> > > I'm all for not wasting time supporting use cases that are not
> > > necessary in practice. We just need to document the expected
> > > behavior clearly, whatever we decide to do.
> >
> > documenting is good, but if it adds new semantics to how CPU features are handled
> > users up the stack will need code it up as well and juggle with
> > -machine + -cpu + -device cpu-foo
> > not to mention poor developers who will have to figure out why we do
> > set CPU properties in multiple different ways.
> >
> > however if we add it as CPU properties that behave the same way as other
> > properties, all mgmt has to do is expose new property to user for usage.
>
> I think we need to be careful here. Sometimes just exposing the
> QOM properties used to implemented a feature is not the best user
> interface. e.g.: even if using compat_props for implementing the
> hyperv features preset, that doesn't automatically mean we want
> hyperv=on to be a -cpu option.
>
> I would even argue we shouldn't be focusing on implementation
> details (like we are doing right now) until the desired external
> interface is described clearly.
>
> >
> > it even more true when building machine from QMP interface would be available,
> > where we would want '-device foo' more or less the same way instead of
> > special casing some of them, i.e. I'd rather have one device to configure,
> > instead of doing it in multiple places. It's not possible in reality
> > but for new code we should try to minimize split brain issues.
>
> Is split brain a practical problem here? If the new behavior is
> implemented in x86_cpu_realizefn() or x86_cpu_pre_plug(), we know
> it's going to affect all CPU objects.
i was talking about user interface here, i.e.:
(QMP) create_machine(hyperv=on)
(QMP) device_add(cpu, hv_foo=x)
vs:
(QMP) device_add(cpu, hyperv_defaults=on,=onhv_foo=x)
i.e. in the later case cpu specific options are consolidate within device stanza
and mgmt doesn't need to be aware and split cpu config in to steps.
> >
> > > >
> > > > > not sure we would like to introduce such invariant,
> > > > > in normal qom property handling the latest set property should have effect
> > > > > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > > > > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > > > > any other QOM object when it come to property handling)
> > > > >
> > > > > anyways it's confusing a bit to have cpu flags to come from 2 different places
> > > > >
> > > > > -cpu hyperv-use-preset=on,hv-foo=off
> > > > >
> > > > > looks less confusing and will heave expected effect
> > > > >
> > > >
> > > > Honestly, 'hyperv-use-preset' is confusing even to me :-)
> > > >
> > > > What if we for a second stop thinking about Hyper-V features being CPU
> > > > features only, e.g. if we want to create Dynamic Memory or PTP or any
> > > > other Hyper-V specific device in a simple way? We'll have to put these
> > > > under machine type.
> > >
> > > I agree. Hyper-V is not just a set of CPU features.
> > me too,
> > however in this case we are talking about a set of cpu features,
> > if there is no way to implement it as cpu properties + compat properties
> > and requires opencodding it within machine code it might be fine
> > but I fail to see a very good reason for doing that at this momment.
>
> The reason would be just simplicity of implementation.
aside other issues,
cpu props + compact_props looks simpler than machine based variant.
>
> I understand there are reasons to suggest using compat_props if
> it makes things simpler, but I don't see why we would reject a
> patch because the implementation is not based purely on
> compat_props.
main issue is that patch introduces new semantics to cpu feature
parsing.
compat_props is for consistency with how we typically handle device
compatibility, which is also good enough reason.
> I will let Vitaly to decide how to proceed, based on our
> feedback. I encourage him to use compat_props like you suggest,
> but I don't plan to make this a requirement.
>
> >
> > >
> > > Also, those two approaches are not mutually exclusive.
> > > "-machine hyperv=on" can be implemented internally using
> > > "hyperv-use-preset=on" if necessary. I don't think it has to,
> > > however.
> >
> >
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 16:03 ` Igor Mammedov
@ 2021-01-05 16:31 ` Vitaly Kuznetsov
2021-01-06 13:13 ` Igor Mammedov
0 siblings, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-01-05 16:31 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
Igor Mammedov <imammedo@redhat.com> writes:
> On Tue, 05 Jan 2021 12:50:05 +0100
>
> I think there is a misunderstanding, idea was:
>
> cpu_initfn() {
> //current set
> cpu->default_hyperv_cpu_features = ACD
> }
>
> compat_props_5.1 {
> cpu.default_hyperv_cpu_features = AB
> }
>
> compat_props_5.2 {
> cpu.default_hyperv_cpu_features = ABC
> }
>
...
> I was talking about CPU features/properties only, it doesn't apply to other devices.
> It makes sense for machine to have a knob to create onboard hyperv specific
> devices if there is any (do we have any?).
>
> If there aren't any currently, I wouldn't bother with machine knob
> and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
> like any other cpu feature.
>
We don't currently have any devices which are not 'CPU features' (in
QEMU terminology), however, we already have Vmbus and I can easily
imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We
*may* want to enable these 'automatically' and that's what make
'-machine' option preferable. It is, however, not a *must* right now and
we can indeed wait until these devices appear and be happy with
'hv_default' -cpu option for now. We will, however, need to teach upper
layers about the change when/if it happens.
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 15:10 ` Vitaly Kuznetsov
@ 2021-01-05 16:33 ` Igor Mammedov
0 siblings, 0 replies; 39+ messages in thread
From: Igor Mammedov @ 2021-01-05 16:33 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
On Tue, 05 Jan 2021 16:10:36 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
> > On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote:
> >>
> >> documenting is good, but if it adds new semantics to how CPU features are handled
> >> users up the stack will need code it up as well and juggle with
> >> -machine + -cpu + -device cpu-foo
> >> not to mention poor developers who will have to figure out why we do
> >> set CPU properties in multiple different ways.
> >>
> >> however if we add it as CPU properties that behave the same way as other
> >> properties, all mgmt has to do is expose new property to user for usage.
> >
> > I think we need to be careful here. Sometimes just exposing the
> > QOM properties used to implemented a feature is not the best user
> > interface. e.g.: even if using compat_props for implementing the
> > hyperv features preset, that doesn't automatically mean we want
> > hyperv=on to be a -cpu option.
> >
> > I would even argue we shouldn't be focusing on implementation
> > details (like we are doing right now) until the desired external
> > interface is described clearly.
>
> I agree, the interface is definitely more important than the
> implementation here. AFAIU we have two options suggested:
>
> 1) 'hyperv=on' option for x86 machine types.
>
> Pros: we can use it later to create non-CPU Hyper-V devices
> (e.g. Vmbus).
> Cons: two different places for the currently existing Hyper-V features
> enablement (-cpu and -machine), non-standard way of doing things
> (code-wise).
>
> 2) 'hv_default=on' -cpu option
>
> Pros: Single place to enable all Hyper-V enlightenments, we can make it
> mutually exclusive with other hv_* options including hv_passthrough
> (clear semantics).
>
> Cons: This can't be reused to create non-CPU objects in the future and
> so upper layers will (again) need to be modified.
>
> There's probably more, please feel free to add.
#1 can be implemented on top of #2, when it becomes necessary.
> >> however in this case we are talking about a set of cpu features,
> >> if there is no way to implement it as cpu properties + compat properties
> >> and requires opencodding it within machine code it might be fine
> >> but I fail to see a very good reason for doing that at this momment.
> >
> > The reason would be just simplicity of implementation.
> >
> > I understand there are reasons to suggest using compat_props if
> > it makes things simpler, but I don't see why we would reject a
> > patch because the implementation is not based purely on
> > compat_props.
> >
> > I will let Vitaly to decide how to proceed, based on our
> > feedback. I encourage him to use compat_props like you suggest,
> > but I don't plan to make this a requirement.
> >
>
> Like I replied to Igor in a parallel thread, I hardly see how using
> compat_props can simplify things in case we decide to keep 'hyperv=on' a
> machine type option. It doesn't seem to fit our use-case when we need a
> mechanism to alter CPU properties for the current machine type as well
> as subtract some features for the old ones. If we, however, decide that
> '-cpu' option is better, then we can try to make it work (but the
> implementation won't be straitforward either).
lets discuss it in that thread.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 16:31 ` Igor Mammedov
@ 2021-01-05 17:02 ` Vitaly Kuznetsov
2021-01-05 18:19 ` Eduardo Habkost
1 sibling, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-01-05 17:02 UTC (permalink / raw)
To: Igor Mammedov, Eduardo Habkost; +Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel
Igor Mammedov <imammedo@redhat.com> writes:
...
>
> i was talking about user interface here, i.e.:
> (QMP) create_machine(hyperv=on)
> (QMP) device_add(cpu, hv_foo=x)
> vs:
> (QMP) device_add(cpu, hyperv_defaults=on,=onhv_foo=x)
>
> i.e. in the later case cpu specific options are consolidate within device stanza
> and mgmt doesn't need to be aware and split cpu config in to steps.
>
FWIW,
'hv_foo=x' only makes sense if 'x' == 'off' as currently we enable all
existing enlightenments. Also, this requires upper layer tools to know
something about 'hv_foo' (to be able to disable it) and AFAICT layers
above libvirt don't actually want to know such low-level details.
Don't get me wrong, I'm not against 'hv_defaults=on', just trying to
give the perspective so we won't need to change the interface again
anytime soon.
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 16:31 ` Igor Mammedov
2021-01-05 17:02 ` Vitaly Kuznetsov
@ 2021-01-05 18:19 ` Eduardo Habkost
1 sibling, 0 replies; 39+ messages in thread
From: Eduardo Habkost @ 2021-01-05 18:19 UTC (permalink / raw)
To: Igor Mammedov
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Tue, Jan 05, 2021 at 05:31:41PM +0100, Igor Mammedov wrote:
> On Tue, 5 Jan 2021 09:34:31 -0500
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote:
> > > On Mon, 4 Jan 2021 13:29:06 -0500
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov wrote:
> > > > > Igor Mammedov <imammedo@redhat.com> writes:
> > > > >
> > > > > >> >
> > > > > >> > + /* Hyper-V features enabled with 'hyperv=on' */
> > > > > >> > + x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> > > > > >> > + BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> > > > > >> > + BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> > > > > >> > + BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> > > > > >> > + BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> > > > > >> > + BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> > > > > >> > + BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> > > > > >> > + BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> > > > > > I'd argue that feature bits do not belong to machine code at all.
> > > > > > If we have to involve machine at all then it should be a set property/value pairs
> > > > > > that machine will set on CPU object (I'm not convinced that doing it
> > > > > > from machine code is good idea though).
> > > > > >
> > > > >
> > > > > These are 'features' and not feature bits. 'Bits' here are just our
> > > > > internal (to QEMU) representation of which features are enable and which
> > > > > are not, we could've just used booleans instead. These feature, when
> > > > > enabled, will result in some CPUID changes (not 1:1) but I don't see how
> > > > > it's different from
> > > > >
> > > > > " -machine q35,accel=kvm "
> > > > >
> > > > > which also results in CPUID changes.
> > > >
> > > > This is a good point, although having accel affect CPUID bits was
> > > > also a source of complexity for query-cpu-model-expansion and
> > > > other QMP queries.
> > >
> > > why was, it's still a headache (mutating CPU models depending on accelerator)
> > >
> > > >
> > > > >
> > > > > The main reason for putting this to x86 machine type is versioning, as
> > > > > we go along we will (hopefully) be implementing more and more Hyper-V
> > > > > features but we want to provide 'one knob to rule them all' but do it in
> > > > > a way that will allow migration. We already have 'hv_passthrough' for
> > > > > CPU.
> > > >
> > > > I agree completely that the set of bits needs to be on
> > > > MachineClass. We just need to agree on the external interface.
> > > That's where I disagree,
> > > let me exaggerate for demo purpose:
> > > - let's move all CPU models feature defaults to MachineClass and forget about compat properties
> > > since in that case we can opencode changes in machine_class_init
> >
> > I don't see your point here. compat_props is also part of
> > MachineClass.
> they are but compat_props are data and we typically use them for
> keeping old behavior for devices, all it needs is adding a line
> to set old property value.
> While class_init is typically used for altering machine specific
> behavior, sure it can be used to patch up device but that's
> a bit more ugly (need to add a field to MachineClass to key off
> and the somehow wire it up to affected device).
I was not excluding compat_props when I said "the set of bits
needs to be on MachineClass", so I don't think we disagree in
this specific point. (Except that I don't think non-compat_props
solution will be necessarily ugly)
>
> > >
> > > It's rather hard code integration between device models, which we try
> > > to avoid and still refactoring QEMU code to get rid of it.
> > > (sure it works until it's not and someone else need to rewrite half of QEMU
> > > to accomplish it's own task because we mixed things together)
> >
> > I don't see why using a X86CPU-specific API that is not based on
> > QOM properties is hard code integration. compat_props is not the
> > only allowed API for machines to communicate with devices.
> >
> > >
> > > >
> > > > >
> > > > > >> >
> > > > > >> > + if (x86ms->hyperv_enabled) {
> > > > > >> > + feat = x86mc->default_hyperv_features;
> > > > > >> > + /* Enlightened VMCS is only available on Intel/VMX */
> > > > > >> > + if (!cpu_has_vmx(&cpu->env)) {
> > > > > >> > + feat &= ~BIT(HYPERV_FEAT_EVMCS);
> > > > > >> > + }
> > > > > >> > +
> > > > > >> > + cpu->hyperv_features |= feat;
> > > > > > that will ignore features user explicitly doesn't want,
> > > > > > ex:
> > > > > > -machine hyperv=on -cpu foo,hv-foo=off
> > > > > >
> > > > >
> > > > > Existing 'hv_passthrough' mode can also affect the result. Personally, I
> > > > > don't see where 'hv-foo=off' is needed outside of debugging and these
> > > > > use-cases can probably be covered by explicitly listing required
> > > > > features but I'm not against making this work, shouldn't be hard.
> > > >
> > > > I'm all for not wasting time supporting use cases that are not
> > > > necessary in practice. We just need to document the expected
> > > > behavior clearly, whatever we decide to do.
> > >
> > > documenting is good, but if it adds new semantics to how CPU features are handled
> > > users up the stack will need code it up as well and juggle with
> > > -machine + -cpu + -device cpu-foo
> > > not to mention poor developers who will have to figure out why we do
> > > set CPU properties in multiple different ways.
> > >
> > > however if we add it as CPU properties that behave the same way as other
> > > properties, all mgmt has to do is expose new property to user for usage.
> >
> > I think we need to be careful here. Sometimes just exposing the
> > QOM properties used to implemented a feature is not the best user
> > interface. e.g.: even if using compat_props for implementing the
> > hyperv features preset, that doesn't automatically mean we want
> > hyperv=on to be a -cpu option.
> >
> > I would even argue we shouldn't be focusing on implementation
> > details (like we are doing right now) until the desired external
> > interface is described clearly.
> >
> > >
> > > it even more true when building machine from QMP interface would be available,
> > > where we would want '-device foo' more or less the same way instead of
> > > special casing some of them, i.e. I'd rather have one device to configure,
> > > instead of doing it in multiple places. It's not possible in reality
> > > but for new code we should try to minimize split brain issues.
> >
> > Is split brain a practical problem here? If the new behavior is
> > implemented in x86_cpu_realizefn() or x86_cpu_pre_plug(), we know
> > it's going to affect all CPU objects.
>
> i was talking about user interface here, i.e.:
> (QMP) create_machine(hyperv=on)
> (QMP) device_add(cpu, hv_foo=x)
> vs:
> (QMP) device_add(cpu, hyperv_defaults=on,=onhv_foo=x)
>
> i.e. in the later case cpu specific options are consolidate within device stanza
> and mgmt doesn't need to be aware and split cpu config in to steps.
This might make sense for this feature. I just worry that one
day we might need to make a machine option to affect CPUID
feature flags, requiring us to make this work somehow. (If we
decide to go with "-cpu hyperv=on", we can postpone that
discussion, though)
>
>
> > >
> > > > >
> > > > > > not sure we would like to introduce such invariant,
> > > > > > in normal qom property handling the latest set property should have effect
> > > > > > (all other invariants we have in x86 cpu property semantics are comming from legacy handling
> > > > > > and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will behave like
> > > > > > any other QOM object when it come to property handling)
> > > > > >
> > > > > > anyways it's confusing a bit to have cpu flags to come from 2 different places
> > > > > >
> > > > > > -cpu hyperv-use-preset=on,hv-foo=off
> > > > > >
> > > > > > looks less confusing and will heave expected effect
> > > > > >
> > > > >
> > > > > Honestly, 'hyperv-use-preset' is confusing even to me :-)
> > > > >
> > > > > What if we for a second stop thinking about Hyper-V features being CPU
> > > > > features only, e.g. if we want to create Dynamic Memory or PTP or any
> > > > > other Hyper-V specific device in a simple way? We'll have to put these
> > > > > under machine type.
> > > >
> > > > I agree. Hyper-V is not just a set of CPU features.
> > > me too,
> > > however in this case we are talking about a set of cpu features,
> > > if there is no way to implement it as cpu properties + compat properties
> > > and requires opencodding it within machine code it might be fine
> > > but I fail to see a very good reason for doing that at this momment.
> >
> > The reason would be just simplicity of implementation.
> aside other issues,
> cpu props + compact_props looks simpler than machine based variant.
Possibly the compat_props solution will be simpler if we choose
the "-cpu hyperv=on" path. I don't expect it to be simpler if we
choose the "-machine hyperv=on" path. Maybe that's our main
source of disagreement (which will go away if we go with
"-cpu hyperv=on").
Both user interface approaches (-cpu -machine) look good enough
to me as long as their behavior is documented and makes sense.
Even better if they have automated test cases.
>
> >
> > I understand there are reasons to suggest using compat_props if
> > it makes things simpler, but I don't see why we would reject a
> > patch because the implementation is not based purely on
> > compat_props.
> main issue is that patch introduces new semantics to cpu feature
> parsing.
> compat_props is for consistency with how we typically handle device
> compatibility, which is also good enough reason.
Is this point about the implementation, or about the user
interface?
>
> > I will let Vitaly to decide how to proceed, based on our
> > feedback. I encourage him to use compat_props like you suggest,
> > but I don't plan to make this a requirement.
> >
> > >
> > > >
> > > > Also, those two approaches are not mutually exclusive.
> > > > "-machine hyperv=on" can be implemented internally using
> > > > "hyperv-use-preset=on" if necessary. I don't think it has to,
> > > > however.
> > >
> > >
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-05 16:31 ` Vitaly Kuznetsov
@ 2021-01-06 13:13 ` Igor Mammedov
2021-01-06 13:38 ` Vitaly Kuznetsov
0 siblings, 1 reply; 39+ messages in thread
From: Igor Mammedov @ 2021-01-06 13:13 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
On Tue, 05 Jan 2021 17:31:43 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> > On Tue, 05 Jan 2021 12:50:05 +0100
> >
> > I think there is a misunderstanding, idea was:
> >
> > cpu_initfn() {
> > //current set
> > cpu->default_hyperv_cpu_features = ACD
> > }
> >
> > compat_props_5.1 {
> > cpu.default_hyperv_cpu_features = AB
> > }
> >
> > compat_props_5.2 {
> > cpu.default_hyperv_cpu_features = ABC
> > }
> >
>
> ...
>
> > I was talking about CPU features/properties only, it doesn't apply to other devices.
> > It makes sense for machine to have a knob to create onboard hyperv specific
> > devices if there is any (do we have any?).
> >
> > If there aren't any currently, I wouldn't bother with machine knob
> > and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
> > like any other cpu feature.
> >
>
> We don't currently have any devices which are not 'CPU features' (in
> QEMU terminology), however, we already have Vmbus and I can easily
> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We
> *may* want to enable these 'automatically' and that's what make
> '-machine' option preferable. It is, however, not a *must* right now and
> we can indeed wait until these devices appear and be happy with
> 'hv_default' -cpu option for now. We will, however, need to teach upper
> layers about the change when/if it happens.
which makes me think we are trying to bite something that we shouldn't.
Do we really need this patch (QEMU knob) to magically enable subset of
features and/or devices for a specific OS flavor?
It's job of upper layers to abstract low level QEMU details in to coarse
grained knobs (libvirt/virt-install/virt-manager/...).
For example virt-install may know that it installing a specific Windows
version, and can build a tailored for that OS configuration including
needed hyperv CPU features and hyperv specific devices.
(if I'm not mistaken libosinfo is used to get metadata for preferred
configuration, so perhaps this should become a patch for that library
and its direct users).
What we actually lack is a documentation for preferred configuration
in docs/hyperv.txt, currently it just enumerates possible features.
We can just document a recommended 'best practices' there without
putting it in QEMU code and let upper layers to do their job in
the stack.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-06 13:13 ` Igor Mammedov
@ 2021-01-06 13:38 ` Vitaly Kuznetsov
2021-01-06 16:45 ` Igor Mammedov
2021-01-06 17:02 ` Eduardo Habkost
0 siblings, 2 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-01-06 13:38 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
Igor Mammedov <imammedo@redhat.com> writes:
> On Tue, 05 Jan 2021 17:31:43 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Igor Mammedov <imammedo@redhat.com> writes:
>>
>> > On Tue, 05 Jan 2021 12:50:05 +0100
>> >
>> > I think there is a misunderstanding, idea was:
>> >
>> > cpu_initfn() {
>> > //current set
>> > cpu->default_hyperv_cpu_features = ACD
>> > }
>> >
>> > compat_props_5.1 {
>> > cpu.default_hyperv_cpu_features = AB
>> > }
>> >
>> > compat_props_5.2 {
>> > cpu.default_hyperv_cpu_features = ABC
>> > }
>> >
>>
>> ...
>>
>> > I was talking about CPU features/properties only, it doesn't apply to other devices.
>> > It makes sense for machine to have a knob to create onboard hyperv specific
>> > devices if there is any (do we have any?).
>> >
>> > If there aren't any currently, I wouldn't bother with machine knob
>> > and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
>> > like any other cpu feature.
>> >
>>
>> We don't currently have any devices which are not 'CPU features' (in
>> QEMU terminology), however, we already have Vmbus and I can easily
>> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We
>> *may* want to enable these 'automatically' and that's what make
>> '-machine' option preferable. It is, however, not a *must* right now and
>> we can indeed wait until these devices appear and be happy with
>> 'hv_default' -cpu option for now. We will, however, need to teach upper
>> layers about the change when/if it happens.
>
> which makes me think we are trying to bite something that we shouldn't.
> Do we really need this patch (QEMU knob) to magically enable subset of
> features and/or devices for a specific OS flavor?
>
> It's job of upper layers to abstract low level QEMU details in to coarse
> grained knobs (libvirt/virt-install/virt-manager/...).
> For example virt-install may know that it installing a specific Windows
> version, and can build a tailored for that OS configuration including
> needed hyperv CPU features and hyperv specific devices.
> (if I'm not mistaken libosinfo is used to get metadata for preferred
> configuration, so perhaps this should become a patch for that library
> and its direct users).
>
> What we actually lack is a documentation for preferred configuration
> in docs/hyperv.txt, currently it just enumerates possible features.
> We can just document a recommended 'best practices' there without
> putting it in QEMU code and let upper layers to do their job in
> the stack.
The problem we're facing here is that when a new enlightenment is
implemented it takes forever to propagate to the whole stack. We don't
have any different recommendations for different Windows versions,
neither does genuine Hyper-V. The 'fine grained' mechanis we have just
contributes to the creation of various Frankenstein configurations
(which look nothing like real Hyper-V), people just google for 'Windows
KVM slow', add something to their scripts and this keeps propagating.
Every time I see a configuration with only a few 'hv_*' options I ask
'why don't you enable the rest?' and I'm yet to receive an answer
different from 'hm, I don't know, I copied it from somewhere and it
worked'.
Setting 'hv_*' options individually should be considered debug only.
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-06 13:38 ` Vitaly Kuznetsov
@ 2021-01-06 16:45 ` Igor Mammedov
2021-01-06 17:25 ` Eduardo Habkost
2021-01-06 17:02 ` Eduardo Habkost
1 sibling, 1 reply; 39+ messages in thread
From: Igor Mammedov @ 2021-01-06 16:45 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost, qemu-devel
On Wed, 06 Jan 2021 14:38:56 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> > On Tue, 05 Jan 2021 17:31:43 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >>
> >> > On Tue, 05 Jan 2021 12:50:05 +0100
> >> >
> >> > I think there is a misunderstanding, idea was:
> >> >
> >> > cpu_initfn() {
> >> > //current set
> >> > cpu->default_hyperv_cpu_features = ACD
> >> > }
> >> >
> >> > compat_props_5.1 {
> >> > cpu.default_hyperv_cpu_features = AB
> >> > }
> >> >
> >> > compat_props_5.2 {
> >> > cpu.default_hyperv_cpu_features = ABC
> >> > }
> >> >
> >>
> >> ...
> >>
> >> > I was talking about CPU features/properties only, it doesn't apply to other devices.
> >> > It makes sense for machine to have a knob to create onboard hyperv specific
> >> > devices if there is any (do we have any?).
> >> >
> >> > If there aren't any currently, I wouldn't bother with machine knob
> >> > and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
> >> > like any other cpu feature.
> >> >
> >>
> >> We don't currently have any devices which are not 'CPU features' (in
> >> QEMU terminology), however, we already have Vmbus and I can easily
> >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We
> >> *may* want to enable these 'automatically' and that's what make
> >> '-machine' option preferable. It is, however, not a *must* right now and
> >> we can indeed wait until these devices appear and be happy with
> >> 'hv_default' -cpu option for now. We will, however, need to teach upper
> >> layers about the change when/if it happens.
> >
> > which makes me think we are trying to bite something that we shouldn't.
> > Do we really need this patch (QEMU knob) to magically enable subset of
> > features and/or devices for a specific OS flavor?
> >
> > It's job of upper layers to abstract low level QEMU details in to coarse
> > grained knobs (libvirt/virt-install/virt-manager/...).
> > For example virt-install may know that it installing a specific Windows
> > version, and can build a tailored for that OS configuration including
> > needed hyperv CPU features and hyperv specific devices.
> > (if I'm not mistaken libosinfo is used to get metadata for preferred
> > configuration, so perhaps this should become a patch for that library
> > and its direct users).
> >
> > What we actually lack is a documentation for preferred configuration
> > in docs/hyperv.txt, currently it just enumerates possible features.
> > We can just document a recommended 'best practices' there without
> > putting it in QEMU code and let upper layers to do their job in
> > the stack.
>
> The problem we're facing here is that when a new enlightenment is
> implemented it takes forever to propagate to the whole stack. We don't
It's true not only for Hyper-V, I guess it's price to pay for modular solution.
> have any different recommendations for different Windows versions,
> neither does genuine Hyper-V. The 'fine grained' mechanis we have just
> contributes to the creation of various Frankenstein configurations
> (which look nothing like real Hyper-V), people just google for 'Windows
> KVM slow', add something to their scripts and this keeps propagating.
That's why I mentioned lack of documentation.
If someone manually configures QEMU, one should understand what they do
enable and why or enlist help of virt-install and likes.
> Every time I see a configuration with only a few 'hv_*' options I ask
> 'why don't you enable the rest?' and I'm yet to receive an answer
> different from 'hm, I don't know, I copied it from somewhere and it
> worked'.
If individual features are are composed by virt-install or other tools
based on libosinfo data, then we don't have to maintain versioning
of new default_set_features per machine type, which will only become
worse if we include hv specific devices into it.
Also with libosinfo approach, old machine types and old QEMU versions
can also benefit from it without need to change whole stack.
And no versioning is necessary since chosen config set is stored in
domain XML at the moment VM is created.
> Setting 'hv_*' options individually should be considered debug only.
that's how cpu's features were designed, a helper knob on top is fine
as long as it doesn't mess the way it used to work and preferably is
build on top of existing features.
PS:
another wild idea how to implement it using '-machine hyperv=on',
based on compat props idea:
// replaces bit set in your version
hv_default_set[] =
"hv_feat1", "hv_feat2",
...
};
// probably should be done before -cpu is parsed
then if machine hyperv=on
foreach in hv_default_set[]
object_register_sugar_prop(hv_default_set[i], "on")
PS2:
my preferred approach is still -cpu hyperv=on, since it doesn't
depend on order CLI is currently parsed (which is fragile thing),
but rather on what user asked us to do with CPU.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-06 13:38 ` Vitaly Kuznetsov
2021-01-06 16:45 ` Igor Mammedov
@ 2021-01-06 17:02 ` Eduardo Habkost
1 sibling, 0 replies; 39+ messages in thread
From: Eduardo Habkost @ 2021-01-06 17:02 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Igor Mammedov, Marcelo Tosatti, qemu-devel, Paolo Bonzini
On Wed, Jan 06, 2021 at 02:38:56PM +0100, Vitaly Kuznetsov wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> > On Tue, 05 Jan 2021 17:31:43 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >>
> >> > On Tue, 05 Jan 2021 12:50:05 +0100
> >> >
> >> > I think there is a misunderstanding, idea was:
> >> >
> >> > cpu_initfn() {
> >> > //current set
> >> > cpu->default_hyperv_cpu_features = ACD
> >> > }
> >> >
> >> > compat_props_5.1 {
> >> > cpu.default_hyperv_cpu_features = AB
> >> > }
> >> >
> >> > compat_props_5.2 {
> >> > cpu.default_hyperv_cpu_features = ABC
> >> > }
> >> >
> >>
> >> ...
> >>
> >> > I was talking about CPU features/properties only, it doesn't apply to other devices.
> >> > It makes sense for machine to have a knob to create onboard hyperv specific
> >> > devices if there is any (do we have any?).
> >> >
> >> > If there aren't any currently, I wouldn't bother with machine knob
> >> > and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
> >> > like any other cpu feature.
> >> >
> >>
> >> We don't currently have any devices which are not 'CPU features' (in
> >> QEMU terminology), however, we already have Vmbus and I can easily
> >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We
> >> *may* want to enable these 'automatically' and that's what make
> >> '-machine' option preferable. It is, however, not a *must* right now and
> >> we can indeed wait until these devices appear and be happy with
> >> 'hv_default' -cpu option for now. We will, however, need to teach upper
> >> layers about the change when/if it happens.
> >
> > which makes me think we are trying to bite something that we shouldn't.
> > Do we really need this patch (QEMU knob) to magically enable subset of
> > features and/or devices for a specific OS flavor?
I think we really want this, yes. It's not for a specific OS
flavor, it is just a machine feature.
> >
> > It's job of upper layers to abstract low level QEMU details in to coarse
> > grained knobs (libvirt/virt-install/virt-manager/...).
> > For example virt-install may know that it installing a specific Windows
> > version, and can build a tailored for that OS configuration including
> > needed hyperv CPU features and hyperv specific devices.
> > (if I'm not mistaken libosinfo is used to get metadata for preferred
> > configuration, so perhaps this should become a patch for that library
> > and its direct users).
virt-install/libosinfo/etc can be used to enable a feature
automatically, but the coarse grained knob may be provided by
QEMU.
> >
> > What we actually lack is a documentation for preferred configuration
> > in docs/hyperv.txt, currently it just enumerates possible features.
> > We can just document a recommended 'best practices' there without
> > putting it in QEMU code and let upper layers to do their job in
> > the stack.
>
> The problem we're facing here is that when a new enlightenment is
> implemented it takes forever to propagate to the whole stack. We don't
> have any different recommendations for different Windows versions,
> neither does genuine Hyper-V. The 'fine grained' mechanis we have just
> contributes to the creation of various Frankenstein configurations
> (which look nothing like real Hyper-V), people just google for 'Windows
> KVM slow', add something to their scripts and this keeps propagating.
Exactly. Requiring new code to be added to all other components
in the stack every time we add a low level feature to KVM or QEMU
is not working. It's even worse when we require users to
manually update their configurations with low level bits.
>
> Every time I see a configuration with only a few 'hv_*' options I ask
> 'why don't you enable the rest?' and I'm yet to receive an answer
> different from 'hm, I don't know, I copied it from somewhere and it
> worked'.
>
> Setting 'hv_*' options individually should be considered debug only.
They can also be useful in production to work around
unexpected issues (not just debugging).
I don't think we should prevent other layers from controlling low
level knobs. We just shouldn't make the low level knobs
necessary for making the feature work.
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-06 16:45 ` Igor Mammedov
@ 2021-01-06 17:25 ` Eduardo Habkost
2021-01-07 9:14 ` Vitaly Kuznetsov
0 siblings, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2021-01-06 17:25 UTC (permalink / raw)
To: Igor Mammedov
Cc: Paolo Bonzini, Vitaly Kuznetsov, Marcelo Tosatti, qemu-devel
On Wed, Jan 06, 2021 at 05:45:42PM +0100, Igor Mammedov wrote:
> On Wed, 06 Jan 2021 14:38:56 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> > Igor Mammedov <imammedo@redhat.com> writes:
> >
> > > On Tue, 05 Jan 2021 17:31:43 +0100
> > > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> > >
> > >> Igor Mammedov <imammedo@redhat.com> writes:
> > >>
> > >> > On Tue, 05 Jan 2021 12:50:05 +0100
> > >> >
> > >> > I think there is a misunderstanding, idea was:
> > >> >
> > >> > cpu_initfn() {
> > >> > //current set
> > >> > cpu->default_hyperv_cpu_features = ACD
> > >> > }
> > >> >
> > >> > compat_props_5.1 {
> > >> > cpu.default_hyperv_cpu_features = AB
> > >> > }
> > >> >
> > >> > compat_props_5.2 {
> > >> > cpu.default_hyperv_cpu_features = ABC
> > >> > }
> > >> >
> > >>
> > >> ...
> > >>
> > >> > I was talking about CPU features/properties only, it doesn't apply to other devices.
> > >> > It makes sense for machine to have a knob to create onboard hyperv specific
> > >> > devices if there is any (do we have any?).
> > >> >
> > >> > If there aren't any currently, I wouldn't bother with machine knob
> > >> > and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
> > >> > like any other cpu feature.
> > >> >
> > >>
> > >> We don't currently have any devices which are not 'CPU features' (in
> > >> QEMU terminology), however, we already have Vmbus and I can easily
> > >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We
> > >> *may* want to enable these 'automatically' and that's what make
> > >> '-machine' option preferable. It is, however, not a *must* right now and
> > >> we can indeed wait until these devices appear and be happy with
> > >> 'hv_default' -cpu option for now. We will, however, need to teach upper
> > >> layers about the change when/if it happens.
> > >
> > > which makes me think we are trying to bite something that we shouldn't.
> > > Do we really need this patch (QEMU knob) to magically enable subset of
> > > features and/or devices for a specific OS flavor?
> > >
> > > It's job of upper layers to abstract low level QEMU details in to coarse
> > > grained knobs (libvirt/virt-install/virt-manager/...).
> > > For example virt-install may know that it installing a specific Windows
> > > version, and can build a tailored for that OS configuration including
> > > needed hyperv CPU features and hyperv specific devices.
> > > (if I'm not mistaken libosinfo is used to get metadata for preferred
> > > configuration, so perhaps this should become a patch for that library
> > > and its direct users).
> > >
> > > What we actually lack is a documentation for preferred configuration
> > > in docs/hyperv.txt, currently it just enumerates possible features.
> > > We can just document a recommended 'best practices' there without
> > > putting it in QEMU code and let upper layers to do their job in
> > > the stack.
> >
> > The problem we're facing here is that when a new enlightenment is
> > implemented it takes forever to propagate to the whole stack. We don't
> It's true not only for Hyper-V, I guess it's price to pay for modular solution.
Yes, this discussion applies to other features as well.
>
> > have any different recommendations for different Windows versions,
> > neither does genuine Hyper-V. The 'fine grained' mechanis we have just
> > contributes to the creation of various Frankenstein configurations
> > (which look nothing like real Hyper-V), people just google for 'Windows
> > KVM slow', add something to their scripts and this keeps propagating.
> That's why I mentioned lack of documentation.
> If someone manually configures QEMU, one should understand what they do
> enable and why or enlist help of virt-install and likes.
Why?
QEMU's lack of usability is an unfortunate accident, not a
desirable goal.
>
> > Every time I see a configuration with only a few 'hv_*' options I ask
> > 'why don't you enable the rest?' and I'm yet to receive an answer
> > different from 'hm, I don't know, I copied it from somewhere and it
> > worked'.
>
> If individual features are are composed by virt-install or other tools
> based on libosinfo data, then we don't have to maintain versioning
> of new default_set_features per machine type, which will only become
> worse if we include hv specific devices into it.
Versioning is extra work for us QEMU developers, but it has a
purpose. It saves everybody else's valuable time.
>
> Also with libosinfo approach, old machine types and old QEMU versions
> can also benefit from it without need to change whole stack.
Except that you need to update the whole stack (QEMU + libvirt +
libosinfo + the glue code between libosinfo and libvirt) every
time a new feature is available.
This is unnecessary overhead, and this is not working.
> And no versioning is necessary since chosen config set is stored in
> domain XML at the moment VM is created.
I don't even think that is a good thing.
I would agree completely with you if the people maintaining the
upper layers were asking us to just let them manage low level
details of guest ABI. They are not.
>
> > Setting 'hv_*' options individually should be considered debug only.
> that's how cpu's features were designed, a helper knob on top is fine
> as long as it doesn't mess the way it used to work and preferably is
> build on top of existing features.
>
> PS:
> another wild idea how to implement it using '-machine hyperv=on',
> based on compat props idea:
>
> // replaces bit set in your version
> hv_default_set[] =
> "hv_feat1", "hv_feat2",
> ...
> };
>
> // probably should be done before -cpu is parsed
> then if machine hyperv=on
> foreach in hv_default_set[]
> object_register_sugar_prop(hv_default_set[i], "on")
This sounds interesting.
>
> PS2:
> my preferred approach is still -cpu hyperv=on, since it doesn't
> depend on order CLI is currently parsed (which is fragile thing),
> but rather on what user asked us to do with CPU.
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
2021-01-06 17:25 ` Eduardo Habkost
@ 2021-01-07 9:14 ` Vitaly Kuznetsov
0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-01-07 9:14 UTC (permalink / raw)
To: Eduardo Habkost, Igor Mammedov; +Cc: Paolo Bonzini, Marcelo Tosatti, qemu-devel
Igor Mammedov wrote:
> my preferred approach is still -cpu hyperv=on, since it doesn't
> depend on order CLI is currently parsed (which is fragile thing),
> but rather on what user asked us to do with CPU.
I think I'm OK with this solution for the time being. When non-CPU
devices arrive and if we decide that it is a good idea to have them
enabled by default, we can make -machine hyperv=on option implying
'hv_default' CPU option. The real benefit I see from -cpu option is
simplification of debug configurations (to find out what would happen
if certain enlightenments are disabled) and making it possible to use
'hv_default' with older kernels lacking some enlightenments (by
disabling them). Not that this is impossible with -machine option, just
not very straitforward ('-cpu host,hv-default,hv-evmcs=off' vs '-machine
q35,hyperv=on -cpu host,hv-evmcs=off').
I'll send out v3 shortly and I'll include patches from "i386: KVM:
expand Hyper-V features early" which were waiting for Linux-5.11 merge
window.
Eduardo Habkost <ehabkost@redhat.com> writes:
>> > On Tue, 05 Jan 2021 17:31:43 +0100
>> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Every time I see a configuration with only a few 'hv_*' options I ask
>> 'why don't you enable the rest?' and I'm yet to receive an answer
>> different from 'hm, I don't know, I copied it from somewhere and it
>> worked'.
>>
>> Setting 'hv_*' options individually should be considered debug only.
>
> They can also be useful in production to work around
> unexpected issues (not just debugging).
>
Right, by 'debugging' I meant 'dealing with issues' :-)
> I don't think we should prevent other layers from controlling low
> level knobs. We just shouldn't make the low level knobs
> necessary for making the feature work.
Let me give an exaggerated example. Why do we have named cpu models
(e.g. 'Skylake')? We could've exposed basic CPU model and all the knobs
to upper layers of the stack to deal with. I don't even want to imagine
the chaos this would've created. Low level knobs are necessary when
issues arise (e.g. it's easy to ask 'could you please try "-cpu
Skylake,-vmx" and see what happens?') but mandating that upper layers
(btw, all of them -- we don't have 'one libvirt to rule them all') have
to set them is non-practical IMO.
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
2020-11-19 10:32 ` [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn() Vitaly Kuznetsov
@ 2021-03-10 11:27 ` Claudio Fontana
2021-03-10 11:43 ` Vitaly Kuznetsov
0 siblings, 1 reply; 39+ messages in thread
From: Claudio Fontana @ 2021-03-10 11:27 UTC (permalink / raw)
To: Vitaly Kuznetsov, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
> As a preparation to expanding Hyper-V CPU features early, move
> hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
> x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
> itself.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Hi Vitaly,
If building for TCG-only, does the x86_cpu_hyperv_realize function and other hyperv related functions
make sense to keep in the build?
Now that we have per-accelerator subdirs in target/i386, should the hyperv code be moved over?
Thanks,
Claudio
> ---
> target/i386/cpu.c | 23 ++++++++++++++++++++++-
> target/i386/cpu.h | 3 ++-
> target/i386/kvm.c | 25 ++++++++++---------------
> 3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5a8c96072e41..2a6885753378 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6509,6 +6509,25 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> }
> }
>
> +static void x86_cpu_hyperv_realize(X86CPU *cpu)
> +{
> + size_t len;
> +
> + /* Hyper-V vendor id */
> + if (!cpu->hyperv_vendor) {
> + memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
> + } else {
> + len = strlen(cpu->hyperv_vendor);
> +
> + if (len > 12) {
> + warn_report("hv-vendor-id truncated to 12 characters");
> + len = 12;
> + }
> + memset(cpu->hyperv_vendor_id, 0, 12);
> + memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
> + }
> +}
> +
> static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> {
> CPUState *cs = CPU(dev);
> @@ -6680,6 +6699,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> env->cache_info_amd.l3_cache = &legacy_l3_cache;
> }
>
> + /* Process Hyper-V enlightenments */
> + x86_cpu_hyperv_realize(cpu);
>
> cpu_exec_realizefn(cs, &local_err);
> if (local_err != NULL) {
> @@ -7198,7 +7219,7 @@ static Property x86_cpu_properties[] = {
> DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
> DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
> DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
> - DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
> + DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
> DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
> DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
> DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 88e8586f8fb4..be640bf45c29 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1655,11 +1655,12 @@ struct X86CPU {
> uint64_t ucode_rev;
>
> uint32_t hyperv_spinlock_attempts;
> - char *hyperv_vendor_id;
> + char *hyperv_vendor;
> bool hyperv_synic_kvm_only;
> uint64_t hyperv_features;
> bool hyperv_passthrough;
> OnOffAuto hyperv_no_nonarch_cs;
> + uint32_t hyperv_vendor_id[3];
>
> bool check_cpuid;
> bool enforce_cpuid;
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index a2934dda027c..788a2cf2ec51 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1205,6 +1205,13 @@ static int hyperv_handle_properties(CPUState *cs,
> memcpy(cpuid_ent, &cpuid->entries[0],
> cpuid->nent * sizeof(cpuid->entries[0]));
>
> + c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
> + if (c) {
> + cpu->hyperv_vendor_id[0] = c->ebx;
> + cpu->hyperv_vendor_id[1] = c->ecx;
> + cpu->hyperv_vendor_id[2] = c->edx;
> + }
> +
> c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
> if (c) {
> env->features[FEAT_HYPERV_EAX] = c->eax;
> @@ -1279,23 +1286,11 @@ static int hyperv_handle_properties(CPUState *cs,
>
> c = &cpuid_ent[cpuid_i++];
> c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
> - if (!cpu->hyperv_vendor_id) {
> - memcpy(signature, "Microsoft Hv", 12);
> - } else {
> - size_t len = strlen(cpu->hyperv_vendor_id);
> -
> - if (len > 12) {
> - error_report("hv-vendor-id truncated to 12 characters");
> - len = 12;
> - }
> - memset(signature, 0, 12);
> - memcpy(signature, cpu->hyperv_vendor_id, len);
> - }
> c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
> HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
> - c->ebx = signature[0];
> - c->ecx = signature[1];
> - c->edx = signature[2];
> + c->ebx = cpu->hyperv_vendor_id[0];
> + c->ecx = cpu->hyperv_vendor_id[1];
> + c->edx = cpu->hyperv_vendor_id[2];
>
> c = &cpuid_ent[cpuid_i++];
> c->function = HV_CPUID_INTERFACE;
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
2021-03-10 11:27 ` Claudio Fontana
@ 2021-03-10 11:43 ` Vitaly Kuznetsov
2021-03-10 12:18 ` Claudio Fontana
0 siblings, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-03-10 11:43 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
Claudio Fontana <cfontana@suse.de> writes:
> On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
>> As a preparation to expanding Hyper-V CPU features early, move
>> hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
>> x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
>> itself.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
>
> Hi Vitaly,
>
> If building for TCG-only, does the x86_cpu_hyperv_realize function and other hyperv related functions
> make sense to keep in the build?
>
> Now that we have per-accelerator subdirs in target/i386, should the hyperv code be moved over?
>
Hi Claudio,
I'm not exactly sure. On one hand, we only implement Hyper-V emulation
with KVM now. On the other hand Hyper-V specific CPU options are
available even without it (and as Igor feels strongly against
introducing custom option parsers, I don't see how we can forbid to use
them without KVM). x86_cpu_hyperv_realize() is the bare minimum which is
only needed to set our internal Hyper-V related data in a sane way,
e.g. set the default to 'cpu->hyperv_vendor_id'. The actual enablement
code is in target/i386/kvm.c already. Do you see anything besides
x86_cpu_hyperv_realize() which we could move there? Could you please
take a look at v5
(https://mail.gnu.org/archive/html/qemu-devel/2021-03/msg00158.html)?
Thanks!
> Thanks,
>
> Claudio
>
>
>> ---
>> target/i386/cpu.c | 23 ++++++++++++++++++++++-
>> target/i386/cpu.h | 3 ++-
>> target/i386/kvm.c | 25 ++++++++++---------------
>> 3 files changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 5a8c96072e41..2a6885753378 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6509,6 +6509,25 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>> }
>> }
>>
>> +static void x86_cpu_hyperv_realize(X86CPU *cpu)
>> +{
>> + size_t len;
>> +
>> + /* Hyper-V vendor id */
>> + if (!cpu->hyperv_vendor) {
>> + memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
>> + } else {
>> + len = strlen(cpu->hyperv_vendor);
>> +
>> + if (len > 12) {
>> + warn_report("hv-vendor-id truncated to 12 characters");
>> + len = 12;
>> + }
>> + memset(cpu->hyperv_vendor_id, 0, 12);
>> + memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
>> + }
>> +}
>> +
>> static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>> {
>> CPUState *cs = CPU(dev);
>> @@ -6680,6 +6699,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>> env->cache_info_amd.l3_cache = &legacy_l3_cache;
>> }
>>
>> + /* Process Hyper-V enlightenments */
>> + x86_cpu_hyperv_realize(cpu);
>>
>> cpu_exec_realizefn(cs, &local_err);
>> if (local_err != NULL) {
>> @@ -7198,7 +7219,7 @@ static Property x86_cpu_properties[] = {
>> DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
>> DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
>> DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
>> - DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
>> + DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
>> DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
>> DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
>> DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index 88e8586f8fb4..be640bf45c29 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1655,11 +1655,12 @@ struct X86CPU {
>> uint64_t ucode_rev;
>>
>> uint32_t hyperv_spinlock_attempts;
>> - char *hyperv_vendor_id;
>> + char *hyperv_vendor;
>> bool hyperv_synic_kvm_only;
>> uint64_t hyperv_features;
>> bool hyperv_passthrough;
>> OnOffAuto hyperv_no_nonarch_cs;
>> + uint32_t hyperv_vendor_id[3];
>>
>> bool check_cpuid;
>> bool enforce_cpuid;
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index a2934dda027c..788a2cf2ec51 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -1205,6 +1205,13 @@ static int hyperv_handle_properties(CPUState *cs,
>> memcpy(cpuid_ent, &cpuid->entries[0],
>> cpuid->nent * sizeof(cpuid->entries[0]));
>>
>> + c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
>> + if (c) {
>> + cpu->hyperv_vendor_id[0] = c->ebx;
>> + cpu->hyperv_vendor_id[1] = c->ecx;
>> + cpu->hyperv_vendor_id[2] = c->edx;
>> + }
>> +
>> c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
>> if (c) {
>> env->features[FEAT_HYPERV_EAX] = c->eax;
>> @@ -1279,23 +1286,11 @@ static int hyperv_handle_properties(CPUState *cs,
>>
>> c = &cpuid_ent[cpuid_i++];
>> c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
>> - if (!cpu->hyperv_vendor_id) {
>> - memcpy(signature, "Microsoft Hv", 12);
>> - } else {
>> - size_t len = strlen(cpu->hyperv_vendor_id);
>> -
>> - if (len > 12) {
>> - error_report("hv-vendor-id truncated to 12 characters");
>> - len = 12;
>> - }
>> - memset(signature, 0, 12);
>> - memcpy(signature, cpu->hyperv_vendor_id, len);
>> - }
>> c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
>> HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
>> - c->ebx = signature[0];
>> - c->ecx = signature[1];
>> - c->edx = signature[2];
>> + c->ebx = cpu->hyperv_vendor_id[0];
>> + c->ecx = cpu->hyperv_vendor_id[1];
>> + c->edx = cpu->hyperv_vendor_id[2];
>>
>> c = &cpuid_ent[cpuid_i++];
>> c->function = HV_CPUID_INTERFACE;
>>
>
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
2021-03-10 11:43 ` Vitaly Kuznetsov
@ 2021-03-10 12:18 ` Claudio Fontana
2021-03-10 13:13 ` Vitaly Kuznetsov
0 siblings, 1 reply; 39+ messages in thread
From: Claudio Fontana @ 2021-03-10 12:18 UTC (permalink / raw)
To: Vitaly Kuznetsov, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
On 3/10/21 12:43 PM, Vitaly Kuznetsov wrote:
> Claudio Fontana <cfontana@suse.de> writes:
>
>> On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
>>> As a preparation to expanding Hyper-V CPU features early, move
>>> hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
>>> x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
>>> itself.
>>>
>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>
>>
>> Hi Vitaly,
>>
>> If building for TCG-only, does the x86_cpu_hyperv_realize function and other hyperv related functions
>> make sense to keep in the build?
>>
>> Now that we have per-accelerator subdirs in target/i386, should the hyperv code be moved over?
>>
>
> Hi Claudio,
>
> I'm not exactly sure. On one hand, we only implement Hyper-V emulation
> with KVM now.
Hi Vitaly,
> On the other hand Hyper-V specific CPU options are
> available even without it (and as Igor feels strongly against
> introducing custom option parsers, I don't see how we can forbid to use
> them without KVM). x86_cpu_hyperv_realize() is the bare minimum which is
> only needed to set our internal Hyper-V related data in a sane way,
> e.g. set the default to 'cpu->hyperv_vendor_id'. The actual enablement
> code is in target/i386/kvm.c already. Do you see anything besides
> x86_cpu_hyperv_realize() which we could move there?
Currently I don't,
I see a lot of PROPs (hyperv_features), but I assume those are the ones you mention cannot be moved right?
> Could you please
> take a look at v5
> (https://mail.gnu.org/archive/html/qemu-devel/2021-03/msg00158.html)?
I took a look at the series you pointed me to, and it seems it is all pretty much kvm/ work to me.
Which hypervisors do you think would theoretically make sense when looking at hyperv emulation in the future?
Looking at the current code and your patches, in the series you link to,
I'd be tempted to suggest moving the hyperv realize function inside kvm_cpu_realizefn in kvm/kvm-cpu.c .
Ciao,
Claudio
>
> Thanks!
>
>
>> Thanks,
>>
>> Claudio
>>
>>
>>> ---
>>> target/i386/cpu.c | 23 ++++++++++++++++++++++-
>>> target/i386/cpu.h | 3 ++-
>>> target/i386/kvm.c | 25 ++++++++++---------------
>>> 3 files changed, 34 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index 5a8c96072e41..2a6885753378 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -6509,6 +6509,25 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>>> }
>>> }
>>>
>>> +static void x86_cpu_hyperv_realize(X86CPU *cpu)
>>> +{
>>> + size_t len;
>>> +
>>> + /* Hyper-V vendor id */
>>> + if (!cpu->hyperv_vendor) {
>>> + memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
>>> + } else {
>>> + len = strlen(cpu->hyperv_vendor);
>>> +
>>> + if (len > 12) {
>>> + warn_report("hv-vendor-id truncated to 12 characters");
>>> + len = 12;
>>> + }
>>> + memset(cpu->hyperv_vendor_id, 0, 12);
>>> + memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
>>> + }
>>> +}
>>> +
>>> static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>> {
>>> CPUState *cs = CPU(dev);
>>> @@ -6680,6 +6699,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>> env->cache_info_amd.l3_cache = &legacy_l3_cache;
>>> }
>>>
>>> + /* Process Hyper-V enlightenments */
>>> + x86_cpu_hyperv_realize(cpu);
>>>
>>> cpu_exec_realizefn(cs, &local_err);
>>> if (local_err != NULL) {
>>> @@ -7198,7 +7219,7 @@ static Property x86_cpu_properties[] = {
>>> DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
>>> DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
>>> DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
>>> - DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
>>> + DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
>>> DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
>>> DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
>>> DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
>>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>>> index 88e8586f8fb4..be640bf45c29 100644
>>> --- a/target/i386/cpu.h
>>> +++ b/target/i386/cpu.h
>>> @@ -1655,11 +1655,12 @@ struct X86CPU {
>>> uint64_t ucode_rev;
>>>
>>> uint32_t hyperv_spinlock_attempts;
>>> - char *hyperv_vendor_id;
>>> + char *hyperv_vendor;
>>> bool hyperv_synic_kvm_only;
>>> uint64_t hyperv_features;
>>> bool hyperv_passthrough;
>>> OnOffAuto hyperv_no_nonarch_cs;
>>> + uint32_t hyperv_vendor_id[3];
>>>
>>> bool check_cpuid;
>>> bool enforce_cpuid;
>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>>> index a2934dda027c..788a2cf2ec51 100644
>>> --- a/target/i386/kvm.c
>>> +++ b/target/i386/kvm.c
>>> @@ -1205,6 +1205,13 @@ static int hyperv_handle_properties(CPUState *cs,
>>> memcpy(cpuid_ent, &cpuid->entries[0],
>>> cpuid->nent * sizeof(cpuid->entries[0]));
>>>
>>> + c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
>>> + if (c) {
>>> + cpu->hyperv_vendor_id[0] = c->ebx;
>>> + cpu->hyperv_vendor_id[1] = c->ecx;
>>> + cpu->hyperv_vendor_id[2] = c->edx;
>>> + }
>>> +
>>> c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
>>> if (c) {
>>> env->features[FEAT_HYPERV_EAX] = c->eax;
>>> @@ -1279,23 +1286,11 @@ static int hyperv_handle_properties(CPUState *cs,
>>>
>>> c = &cpuid_ent[cpuid_i++];
>>> c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
>>> - if (!cpu->hyperv_vendor_id) {
>>> - memcpy(signature, "Microsoft Hv", 12);
>>> - } else {
>>> - size_t len = strlen(cpu->hyperv_vendor_id);
>>> -
>>> - if (len > 12) {
>>> - error_report("hv-vendor-id truncated to 12 characters");
>>> - len = 12;
>>> - }
>>> - memset(signature, 0, 12);
>>> - memcpy(signature, cpu->hyperv_vendor_id, len);
>>> - }
>>> c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
>>> HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
>>> - c->ebx = signature[0];
>>> - c->ecx = signature[1];
>>> - c->edx = signature[2];
>>> + c->ebx = cpu->hyperv_vendor_id[0];
>>> + c->ecx = cpu->hyperv_vendor_id[1];
>>> + c->edx = cpu->hyperv_vendor_id[2];
>>>
>>> c = &cpuid_ent[cpuid_i++];
>>> c->function = HV_CPUID_INTERFACE;
>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
2021-03-10 12:18 ` Claudio Fontana
@ 2021-03-10 13:13 ` Vitaly Kuznetsov
0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2021-03-10 13:13 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel
Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost
Claudio Fontana <cfontana@suse.de> writes:
> On 3/10/21 12:43 PM, Vitaly Kuznetsov wrote:
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
>>>> As a preparation to expanding Hyper-V CPU features early, move
>>>> hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
>>>> x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
>>>> itself.
>>>>
>>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>>
>>>
>>> Hi Vitaly,
>>>
>>> If building for TCG-only, does the x86_cpu_hyperv_realize function and other hyperv related functions
>>> make sense to keep in the build?
>>>
>>> Now that we have per-accelerator subdirs in target/i386, should the hyperv code be moved over?
>>>
>>
>> Hi Claudio,
>>
>> I'm not exactly sure. On one hand, we only implement Hyper-V emulation
>> with KVM now.
>
>
> Hi Vitaly,
>
>
>> On the other hand Hyper-V specific CPU options are
>> available even without it (and as Igor feels strongly against
>> introducing custom option parsers, I don't see how we can forbid to use
>> them without KVM). x86_cpu_hyperv_realize() is the bare minimum which is
>> only needed to set our internal Hyper-V related data in a sane way,
>> e.g. set the default to 'cpu->hyperv_vendor_id'. The actual enablement
>> code is in target/i386/kvm.c already. Do you see anything besides
>> x86_cpu_hyperv_realize() which we could move there?
>
> Currently I don't,
> I see a lot of PROPs (hyperv_features), but I assume those are the ones you mention cannot be moved right?
>
Right, not without introducing a custom parser checking if KVM is in use.
>
>> Could you please
>> take a look at v5
>> (https://mail.gnu.org/archive/html/qemu-devel/2021-03/msg00158.html)?
>
>
> I took a look at the series you pointed me to, and it seems it is all pretty much kvm/ work to me.
>
> Which hypervisors do you think would theoretically make sense when looking at hyperv emulation in the future?
>
In theory, I don't see why we can't emulate hyper-v in TCG, I'm just not
sure how practical that would be). There was a Windows Hypervisor
Platform accelerator (whpx) which uses genuine Hyper-V and can use
Hyper-V enlightenment option to set guest visible CPUIDs but honestly I
don't know much about whpx.
> Looking at the current code and your patches, in the series you link to,
> I'd be tempted to suggest moving the hyperv realize function inside
> kvm_cpu_realizefn in kvm/kvm-cpu.c .
No problem, I wouldn't be against that. I'm waiting for some feedback on
v5 and if I am to do v6 I'll include that. In case I got luck and v5 is
to get merged, I can send a follow-up patch.
>
> Ciao,
>
> Claudio
>
>
>
>>
>> Thanks!
>>
>>
>>> Thanks,
>>>
>>> Claudio
>>>
>>>
>>>> ---
>>>> target/i386/cpu.c | 23 ++++++++++++++++++++++-
>>>> target/i386/cpu.h | 3 ++-
>>>> target/i386/kvm.c | 25 ++++++++++---------------
>>>> 3 files changed, 34 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>>> index 5a8c96072e41..2a6885753378 100644
>>>> --- a/target/i386/cpu.c
>>>> +++ b/target/i386/cpu.c
>>>> @@ -6509,6 +6509,25 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>>>> }
>>>> }
>>>>
>>>> +static void x86_cpu_hyperv_realize(X86CPU *cpu)
>>>> +{
>>>> + size_t len;
>>>> +
>>>> + /* Hyper-V vendor id */
>>>> + if (!cpu->hyperv_vendor) {
>>>> + memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
>>>> + } else {
>>>> + len = strlen(cpu->hyperv_vendor);
>>>> +
>>>> + if (len > 12) {
>>>> + warn_report("hv-vendor-id truncated to 12 characters");
>>>> + len = 12;
>>>> + }
>>>> + memset(cpu->hyperv_vendor_id, 0, 12);
>>>> + memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
>>>> + }
>>>> +}
>>>> +
>>>> static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>>> {
>>>> CPUState *cs = CPU(dev);
>>>> @@ -6680,6 +6699,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>>> env->cache_info_amd.l3_cache = &legacy_l3_cache;
>>>> }
>>>>
>>>> + /* Process Hyper-V enlightenments */
>>>> + x86_cpu_hyperv_realize(cpu);
>>>>
>>>> cpu_exec_realizefn(cs, &local_err);
>>>> if (local_err != NULL) {
>>>> @@ -7198,7 +7219,7 @@ static Property x86_cpu_properties[] = {
>>>> DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
>>>> DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
>>>> DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
>>>> - DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
>>>> + DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
>>>> DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
>>>> DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
>>>> DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
>>>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>>>> index 88e8586f8fb4..be640bf45c29 100644
>>>> --- a/target/i386/cpu.h
>>>> +++ b/target/i386/cpu.h
>>>> @@ -1655,11 +1655,12 @@ struct X86CPU {
>>>> uint64_t ucode_rev;
>>>>
>>>> uint32_t hyperv_spinlock_attempts;
>>>> - char *hyperv_vendor_id;
>>>> + char *hyperv_vendor;
>>>> bool hyperv_synic_kvm_only;
>>>> uint64_t hyperv_features;
>>>> bool hyperv_passthrough;
>>>> OnOffAuto hyperv_no_nonarch_cs;
>>>> + uint32_t hyperv_vendor_id[3];
>>>>
>>>> bool check_cpuid;
>>>> bool enforce_cpuid;
>>>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>>>> index a2934dda027c..788a2cf2ec51 100644
>>>> --- a/target/i386/kvm.c
>>>> +++ b/target/i386/kvm.c
>>>> @@ -1205,6 +1205,13 @@ static int hyperv_handle_properties(CPUState *cs,
>>>> memcpy(cpuid_ent, &cpuid->entries[0],
>>>> cpuid->nent * sizeof(cpuid->entries[0]));
>>>>
>>>> + c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
>>>> + if (c) {
>>>> + cpu->hyperv_vendor_id[0] = c->ebx;
>>>> + cpu->hyperv_vendor_id[1] = c->ecx;
>>>> + cpu->hyperv_vendor_id[2] = c->edx;
>>>> + }
>>>> +
>>>> c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
>>>> if (c) {
>>>> env->features[FEAT_HYPERV_EAX] = c->eax;
>>>> @@ -1279,23 +1286,11 @@ static int hyperv_handle_properties(CPUState *cs,
>>>>
>>>> c = &cpuid_ent[cpuid_i++];
>>>> c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
>>>> - if (!cpu->hyperv_vendor_id) {
>>>> - memcpy(signature, "Microsoft Hv", 12);
>>>> - } else {
>>>> - size_t len = strlen(cpu->hyperv_vendor_id);
>>>> -
>>>> - if (len > 12) {
>>>> - error_report("hv-vendor-id truncated to 12 characters");
>>>> - len = 12;
>>>> - }
>>>> - memset(signature, 0, 12);
>>>> - memcpy(signature, cpu->hyperv_vendor_id, len);
>>>> - }
>>>> c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
>>>> HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
>>>> - c->ebx = signature[0];
>>>> - c->ecx = signature[1];
>>>> - c->edx = signature[2];
>>>> + c->ebx = cpu->hyperv_vendor_id[0];
>>>> + c->ecx = cpu->hyperv_vendor_id[1];
>>>> + c->edx = cpu->hyperv_vendor_id[2];
>>>>
>>>> c = &cpuid_ent[cpuid_i++];
>>>> c->function = HV_CPUID_INTERFACE;
>>>>
>>>
>>
>
--
Vitaly
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2021-03-10 13:15 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 10:32 [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn() Vitaly Kuznetsov
2021-03-10 11:27 ` Claudio Fontana
2021-03-10 11:43 ` Vitaly Kuznetsov
2021-03-10 12:18 ` Claudio Fontana
2021-03-10 13:13 ` Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 2/5] i386: move hyperv_interface_id " Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 3/5] i386: move hyperv_version_id " Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 4/5] i386: move hyperv_limits " Vitaly Kuznetsov
2020-11-19 10:32 ` [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types Vitaly Kuznetsov
2020-12-16 20:52 ` Eduardo Habkost
2020-12-17 9:34 ` Vitaly Kuznetsov
2020-12-18 17:13 ` Igor Mammedov
2020-12-18 18:07 ` Eduardo Habkost
2020-12-21 13:24 ` Igor Mammedov
2020-12-21 19:47 ` Eduardo Habkost
2020-12-21 20:39 ` David Hildenbrand
2021-01-04 12:54 ` Vitaly Kuznetsov
2021-01-04 18:29 ` Eduardo Habkost
2021-01-04 23:36 ` Igor Mammedov
2021-01-05 14:34 ` Eduardo Habkost
2021-01-05 15:10 ` Vitaly Kuznetsov
2021-01-05 16:33 ` Igor Mammedov
2021-01-05 16:31 ` Igor Mammedov
2021-01-05 17:02 ` Vitaly Kuznetsov
2021-01-05 18:19 ` Eduardo Habkost
2021-01-04 23:04 ` Igor Mammedov
2021-01-05 11:50 ` Vitaly Kuznetsov
2021-01-05 16:03 ` Igor Mammedov
2021-01-05 16:31 ` Vitaly Kuznetsov
2021-01-06 13:13 ` Igor Mammedov
2021-01-06 13:38 ` Vitaly Kuznetsov
2021-01-06 16:45 ` Igor Mammedov
2021-01-06 17:25 ` Eduardo Habkost
2021-01-07 9:14 ` Vitaly Kuznetsov
2021-01-06 17:02 ` Eduardo Habkost
2020-11-19 14:22 ` [PATCH 0/5] i386: simplify Hyper-V enlightenments enablement Claudio Fontana
2020-11-19 16:58 ` Vitaly Kuznetsov
2020-12-16 19:09 ` Eduardo Habkost
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.