From: Eduardo Habkost <ehabkost@redhat.com> To: Borislav Petkov <bp@alien8.de> Cc: LKML <linux-kernel@vger.kernel.org>, Borislav Petkov <bp@suse.de>, "H. Peter Anvin" <hpa@zytor.com>, Gleb Natapov <gleb@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Andre Przywara <andre@andrep.de>, Joerg Roedel <joro@8bytes.org>, X86 ML <x86@kernel.org>, KVM <kvm@vger.kernel.org>, qemu-devel@nongnu.org, libvir-list@redhat.com, Jiri Denemark <jdenemar@redhat.com> Subject: Re: [PATCH 6/6] qemu: Add support for emulated CPU features Date: Mon, 23 Sep 2013 14:06:48 -0300 [thread overview] Message-ID: <20130923170648.GD7264@otherpad.lan.raisama.net> (raw) In-Reply-To: <1379861095-628-7-git-send-email-bp@alien8.de> (CCing qemu-devel and libvir-list) On Sun, Sep 22, 2013 at 04:44:55PM +0200, Borislav Petkov wrote: > From: Borislav Petkov <bp@suse.de> > > Add support for the KVM_GET_EMULATED_CPUID ioctl and leave feature bits > enabled, when requested by userspace, if kvm emulates them. > > Signed-off-by: Borislav Petkov <bp@suse.de> This will break some assumptions about the semantics of "-cpu host", that today means "enable every single feature that can be enabled in the host". This, in turn, breaks the assumption that "-cpu host" can be used by libvirt to get information about all CPU features supported by the host. So, I have two questions: 1) Should "-cpu host" enable GET_EMULATED_CPUID features? (I believe the answer is "no"); 2) If not, we need a new mechanism to let libvirt know which features can be enabled on a host (in a way that would allow libvirt to easily calculate the set of possible destination hosts to which a VM can be migrated, before deciding to migrate it). This interface is getting complex enough for me to consider simply asking libvirt developers to call GET_EMULATED_CPUID and GET_SUPPORTED_CPUID directly, instead of asking QEMU for information. But if libvirt does that, it needs a way to know if it's running a QEMU version that supports GET_EMULATED_CPUID, or an old version that supports only GET_SUPPORTED_CPUID. Another alternative is to simply let libvirt ignore GET_EMULATED_CPUID by now, and treat GET_EMULATED_CPUID features as not supported by the host. But then it will break the use cases that prompted the creation of GET_EMULATED_CPUID in the first place. > --- > include/sysemu/kvm.h | 4 ++++ > linux-headers/linux/kvm.h | 4 ++++ > target-i386/cpu.c | 7 +++++++ > target-i386/kvm.c | 38 ++++++++++++++++++++++++++++++++++---- > 4 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 9bbe3db1464e..8eda1ada848a 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -265,6 +265,10 @@ int kvm_check_extension(KVMState *s, unsigned int extension); > > uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, > uint32_t index, int reg); > + > +uint32_t kvm_arch_get_emulated_cpuid(KVMState *env, uint32_t function, > + uint32_t index, int reg); > + > void kvm_cpu_synchronize_state(CPUState *cpu); > > /* generic hooks - to be moved/refactored once there are more users */ > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > index c614070662e1..edc8f2db1f8d 100644 > --- a/linux-headers/linux/kvm.h > +++ b/linux-headers/linux/kvm.h > @@ -541,6 +541,7 @@ struct kvm_ppc_smmu_info { > #define KVM_TRACE_ENABLE __KVM_DEPRECATED_MAIN_W_0x06 > #define KVM_TRACE_PAUSE __KVM_DEPRECATED_MAIN_0x07 > #define KVM_TRACE_DISABLE __KVM_DEPRECATED_MAIN_0x08 > +#define KVM_GET_EMULATED_CPUID _IOWR(KVMIO, 0x09, struct kvm_cpuid2) > > /* > * Extension capability list. > @@ -666,6 +667,9 @@ struct kvm_ppc_smmu_info { > #define KVM_CAP_IRQ_MPIC 90 > #define KVM_CAP_PPC_RTAS 91 > #define KVM_CAP_IRQ_XICS 92 > +#define KVM_CAP_ARM_EL1_32BIT 93 > +#define KVM_CAP_SPAPR_MULTITCE 94 > +#define KVM_CAP_EXT_EMUL_CPUID 95 > > #ifdef KVM_CAP_IRQ_ROUTING > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index c36345e426d7..5406348ceb4a 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1850,7 +1850,14 @@ static void filter_features_for_kvm(X86CPU *cpu) > wi->cpuid_ecx, > wi->cpuid_reg); > uint32_t requested_features = env->features[w]; > + > + uint32_t emul_features = kvm_arch_get_emulated_cpuid(s, wi->cpuid_eax, > + wi->cpuid_ecx, > + wi->cpuid_reg); > + > env->features[w] &= host_feat; > + env->features[w] |= (requested_features & emul_features); > + I find the above logic confusing: you remove a few bits just to re-add them later. Isn't easier and simpler to simply do: env->features[w] &= (host_feat | emul_features); ? > cpu->filtered_features[w] = requested_features & ~env->features[w]; > } > } > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 749aa09a21a6..7f598f01bda5 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -80,7 +80,7 @@ bool kvm_allows_irq0_override(void) > return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing(); > } > > -static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) > +static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int ioctl, int max) > { > struct kvm_cpuid2 *cpuid; > int r, size; > @@ -88,7 +88,7 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) > size = sizeof(*cpuid) + max * sizeof(*cpuid->entries); > cpuid = (struct kvm_cpuid2 *)g_malloc0(size); > cpuid->nent = max; > - r = kvm_ioctl(s, KVM_GET_SUPPORTED_CPUID, cpuid); > + r = kvm_ioctl(s, ioctl, cpuid); > if (r == 0 && cpuid->nent >= max) { > r = -E2BIG; > } > @@ -97,7 +97,10 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) > g_free(cpuid); > return NULL; > } else { > - fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n", > + fprintf(stderr, "%s failed: %s\n", > + (ioctl == (int)KVM_GET_SUPPORTED_CPUID > + ? "KVM_GET_SUPPORTED_CPUID" > + : "KVM_GET_EMULATED_CPUID"), > strerror(-r)); > exit(1); > } > @@ -112,7 +115,17 @@ static struct kvm_cpuid2 *get_supported_cpuid(KVMState *s) > { > struct kvm_cpuid2 *cpuid; > int max = 1; > - while ((cpuid = try_get_cpuid(s, max)) == NULL) { > + while ((cpuid = try_get_cpuid(s, KVM_GET_SUPPORTED_CPUID, max)) == NULL) { > + max *= 2; > + } > + return cpuid; > +} > + > +static struct kvm_cpuid2 *get_emulated_cpuid(KVMState *s) > +{ > + struct kvm_cpuid2 *cpuid; > + int max = 1; > + while ((cpuid = try_get_cpuid(s, KVM_GET_EMULATED_CPUID, max)) == NULL) { > max *= 2; > } > return cpuid; > @@ -241,6 +254,23 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > return ret; > } > > +uint32_t kvm_arch_get_emulated_cpuid(KVMState *s, uint32_t function, > + uint32_t index, int reg) > +{ > + struct kvm_cpuid2 *cpuid __attribute__((unused)); > + > + if (!kvm_check_extension(s, KVM_CAP_EXT_EMUL_CPUID)) > + return 0; > + > + cpuid = get_emulated_cpuid(s); > + > + struct kvm_cpuid_entry2 *entry = cpuid_find_entry(cpuid, function, index); > + if (entry) > + return cpuid_entry_get_reg(entry, reg); > + > + return 0; > +} > + > typedef struct HWPoisonPage { > ram_addr_t ram_addr; > QLIST_ENTRY(HWPoisonPage) list; > -- > 1.8.4 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Eduardo
WARNING: multiple messages have this Message-ID (diff)
From: Eduardo Habkost <ehabkost@redhat.com> To: Borislav Petkov <bp@alien8.de> Cc: KVM <kvm@vger.kernel.org>, Gleb Natapov <gleb@redhat.com>, libvir-list@redhat.com, Joerg Roedel <joro@8bytes.org>, X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>, qemu-devel@nongnu.org, Andre Przywara <andre@andrep.de>, "H. Peter Anvin" <hpa@zytor.com>, Paolo Bonzini <pbonzini@redhat.com>, Jiri Denemark <jdenemar@redhat.com>, Borislav Petkov <bp@suse.de> Subject: Re: [Qemu-devel] [PATCH 6/6] qemu: Add support for emulated CPU features Date: Mon, 23 Sep 2013 14:06:48 -0300 [thread overview] Message-ID: <20130923170648.GD7264@otherpad.lan.raisama.net> (raw) In-Reply-To: <1379861095-628-7-git-send-email-bp@alien8.de> (CCing qemu-devel and libvir-list) On Sun, Sep 22, 2013 at 04:44:55PM +0200, Borislav Petkov wrote: > From: Borislav Petkov <bp@suse.de> > > Add support for the KVM_GET_EMULATED_CPUID ioctl and leave feature bits > enabled, when requested by userspace, if kvm emulates them. > > Signed-off-by: Borislav Petkov <bp@suse.de> This will break some assumptions about the semantics of "-cpu host", that today means "enable every single feature that can be enabled in the host". This, in turn, breaks the assumption that "-cpu host" can be used by libvirt to get information about all CPU features supported by the host. So, I have two questions: 1) Should "-cpu host" enable GET_EMULATED_CPUID features? (I believe the answer is "no"); 2) If not, we need a new mechanism to let libvirt know which features can be enabled on a host (in a way that would allow libvirt to easily calculate the set of possible destination hosts to which a VM can be migrated, before deciding to migrate it). This interface is getting complex enough for me to consider simply asking libvirt developers to call GET_EMULATED_CPUID and GET_SUPPORTED_CPUID directly, instead of asking QEMU for information. But if libvirt does that, it needs a way to know if it's running a QEMU version that supports GET_EMULATED_CPUID, or an old version that supports only GET_SUPPORTED_CPUID. Another alternative is to simply let libvirt ignore GET_EMULATED_CPUID by now, and treat GET_EMULATED_CPUID features as not supported by the host. But then it will break the use cases that prompted the creation of GET_EMULATED_CPUID in the first place. > --- > include/sysemu/kvm.h | 4 ++++ > linux-headers/linux/kvm.h | 4 ++++ > target-i386/cpu.c | 7 +++++++ > target-i386/kvm.c | 38 ++++++++++++++++++++++++++++++++++---- > 4 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 9bbe3db1464e..8eda1ada848a 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -265,6 +265,10 @@ int kvm_check_extension(KVMState *s, unsigned int extension); > > uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, > uint32_t index, int reg); > + > +uint32_t kvm_arch_get_emulated_cpuid(KVMState *env, uint32_t function, > + uint32_t index, int reg); > + > void kvm_cpu_synchronize_state(CPUState *cpu); > > /* generic hooks - to be moved/refactored once there are more users */ > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h > index c614070662e1..edc8f2db1f8d 100644 > --- a/linux-headers/linux/kvm.h > +++ b/linux-headers/linux/kvm.h > @@ -541,6 +541,7 @@ struct kvm_ppc_smmu_info { > #define KVM_TRACE_ENABLE __KVM_DEPRECATED_MAIN_W_0x06 > #define KVM_TRACE_PAUSE __KVM_DEPRECATED_MAIN_0x07 > #define KVM_TRACE_DISABLE __KVM_DEPRECATED_MAIN_0x08 > +#define KVM_GET_EMULATED_CPUID _IOWR(KVMIO, 0x09, struct kvm_cpuid2) > > /* > * Extension capability list. > @@ -666,6 +667,9 @@ struct kvm_ppc_smmu_info { > #define KVM_CAP_IRQ_MPIC 90 > #define KVM_CAP_PPC_RTAS 91 > #define KVM_CAP_IRQ_XICS 92 > +#define KVM_CAP_ARM_EL1_32BIT 93 > +#define KVM_CAP_SPAPR_MULTITCE 94 > +#define KVM_CAP_EXT_EMUL_CPUID 95 > > #ifdef KVM_CAP_IRQ_ROUTING > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index c36345e426d7..5406348ceb4a 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1850,7 +1850,14 @@ static void filter_features_for_kvm(X86CPU *cpu) > wi->cpuid_ecx, > wi->cpuid_reg); > uint32_t requested_features = env->features[w]; > + > + uint32_t emul_features = kvm_arch_get_emulated_cpuid(s, wi->cpuid_eax, > + wi->cpuid_ecx, > + wi->cpuid_reg); > + > env->features[w] &= host_feat; > + env->features[w] |= (requested_features & emul_features); > + I find the above logic confusing: you remove a few bits just to re-add them later. Isn't easier and simpler to simply do: env->features[w] &= (host_feat | emul_features); ? > cpu->filtered_features[w] = requested_features & ~env->features[w]; > } > } > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index 749aa09a21a6..7f598f01bda5 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -80,7 +80,7 @@ bool kvm_allows_irq0_override(void) > return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing(); > } > > -static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) > +static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int ioctl, int max) > { > struct kvm_cpuid2 *cpuid; > int r, size; > @@ -88,7 +88,7 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) > size = sizeof(*cpuid) + max * sizeof(*cpuid->entries); > cpuid = (struct kvm_cpuid2 *)g_malloc0(size); > cpuid->nent = max; > - r = kvm_ioctl(s, KVM_GET_SUPPORTED_CPUID, cpuid); > + r = kvm_ioctl(s, ioctl, cpuid); > if (r == 0 && cpuid->nent >= max) { > r = -E2BIG; > } > @@ -97,7 +97,10 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) > g_free(cpuid); > return NULL; > } else { > - fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n", > + fprintf(stderr, "%s failed: %s\n", > + (ioctl == (int)KVM_GET_SUPPORTED_CPUID > + ? "KVM_GET_SUPPORTED_CPUID" > + : "KVM_GET_EMULATED_CPUID"), > strerror(-r)); > exit(1); > } > @@ -112,7 +115,17 @@ static struct kvm_cpuid2 *get_supported_cpuid(KVMState *s) > { > struct kvm_cpuid2 *cpuid; > int max = 1; > - while ((cpuid = try_get_cpuid(s, max)) == NULL) { > + while ((cpuid = try_get_cpuid(s, KVM_GET_SUPPORTED_CPUID, max)) == NULL) { > + max *= 2; > + } > + return cpuid; > +} > + > +static struct kvm_cpuid2 *get_emulated_cpuid(KVMState *s) > +{ > + struct kvm_cpuid2 *cpuid; > + int max = 1; > + while ((cpuid = try_get_cpuid(s, KVM_GET_EMULATED_CPUID, max)) == NULL) { > max *= 2; > } > return cpuid; > @@ -241,6 +254,23 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > return ret; > } > > +uint32_t kvm_arch_get_emulated_cpuid(KVMState *s, uint32_t function, > + uint32_t index, int reg) > +{ > + struct kvm_cpuid2 *cpuid __attribute__((unused)); > + > + if (!kvm_check_extension(s, KVM_CAP_EXT_EMUL_CPUID)) > + return 0; > + > + cpuid = get_emulated_cpuid(s); > + > + struct kvm_cpuid_entry2 *entry = cpuid_find_entry(cpuid, function, index); > + if (entry) > + return cpuid_entry_get_reg(entry, reg); > + > + return 0; > +} > + > typedef struct HWPoisonPage { > ram_addr_t ram_addr; > QLIST_ENTRY(HWPoisonPage) list; > -- > 1.8.4 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Eduardo
next prev parent reply other threads:[~2013-09-23 17:07 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-09-22 14:44 [PATCH 0/6] kvm: Emulate MOVBE, v3 Borislav Petkov 2013-09-22 14:44 ` [PATCH 1/6] kvm: Add KVM_GET_EMULATED_CPUID Borislav Petkov 2013-09-23 16:28 ` Eduardo Habkost 2013-09-23 16:28 ` [Qemu-devel] " Eduardo Habkost 2013-09-23 16:28 ` Eduardo Habkost 2013-09-24 9:57 ` Borislav Petkov 2013-09-24 9:57 ` [Qemu-devel] " Borislav Petkov 2013-09-24 10:04 ` Gleb Natapov 2013-09-24 10:04 ` [Qemu-devel] " Gleb Natapov 2013-09-26 14:19 ` Eduardo Habkost 2013-09-26 14:19 ` [Qemu-devel] " Eduardo Habkost 2013-09-26 18:55 ` Borislav Petkov 2013-09-26 18:55 ` [Qemu-devel] " Borislav Petkov 2013-09-26 19:20 ` Eduardo Habkost 2013-09-26 19:20 ` [Qemu-devel] " Eduardo Habkost 2013-09-26 20:32 ` Borislav Petkov 2013-09-26 20:32 ` [Qemu-devel] " Borislav Petkov 2013-09-26 20:32 ` Borislav Petkov 2013-09-27 14:21 ` Eduardo Habkost 2013-09-27 14:21 ` [Qemu-devel] " Eduardo Habkost 2013-09-28 10:49 ` Borislav Petkov 2013-09-28 10:49 ` [Qemu-devel] " Borislav Petkov 2013-09-30 16:13 ` Eduardo Habkost 2013-09-30 16:13 ` [Qemu-devel] " Eduardo Habkost 2013-09-30 16:18 ` Borislav Petkov 2013-09-30 16:18 ` [Qemu-devel] " Borislav Petkov 2013-09-22 14:44 ` [PATCH 2/6] kvm, emulator: Use opcode length Borislav Petkov 2013-09-22 14:44 ` [PATCH 3/6] kvm, emulator: Rename VendorSpecific flag Borislav Petkov 2013-09-22 14:44 ` [PATCH 4/6] kvm, emulator: Add initial three-byte insns support Borislav Petkov 2013-10-29 9:50 ` Gleb Natapov 2013-10-29 10:04 ` Borislav Petkov 2013-10-29 10:11 ` Gleb Natapov 2013-09-22 14:44 ` [PATCH 5/6] kvm: Emulate MOVBE Borislav Petkov 2013-09-22 14:44 ` [PATCH 6/6] qemu: Add support for emulated CPU features Borislav Petkov 2013-09-23 17:06 ` Eduardo Habkost [this message] 2013-09-23 17:06 ` [Qemu-devel] " Eduardo Habkost 2013-10-29 9:53 ` [PATCH 0/6] kvm: Emulate MOVBE, v3 Gleb Natapov 2013-10-29 10:30 ` Borislav Petkov 2013-10-29 10:35 ` Gleb Natapov 2013-10-29 11:28 ` Borislav Petkov 2013-10-29 11:36 ` Gleb Natapov 2013-10-29 11:53 ` Borislav Petkov 2013-10-29 11:54 ` [PATCH 4/5 -v3.1] kvm, emulator: Add initial three-byte insns support Borislav Petkov 2013-10-29 11:54 ` [PATCH 5/5 -v3.1] kvm: Emulate MOVBE Borislav Petkov 2013-10-30 17:56 ` Paolo Bonzini 2013-10-30 18:03 ` Borislav Petkov 2013-10-30 18:10 ` [PATCH 0/6] kvm: Emulate MOVBE, v3 Paolo Bonzini 2013-10-30 18:44 ` Borislav Petkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130923170648.GD7264@otherpad.lan.raisama.net \ --to=ehabkost@redhat.com \ --cc=andre@andrep.de \ --cc=bp@alien8.de \ --cc=bp@suse.de \ --cc=gleb@redhat.com \ --cc=hpa@zytor.com \ --cc=jdenemar@redhat.com \ --cc=joro@8bytes.org \ --cc=kvm@vger.kernel.org \ --cc=libvir-list@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.